Repository navigation
feat(phone-conversation): Twilio setup from the chat (numbers, buy, webhook) - #4666
Conversation
…webhook Owner feedback (2026-09-20): after signing up for Twilio the person was sent to the Twilio console to buy a number and paste the webhook URL by hand, and again every time the ngrok URL moved (startup.sh could only print "update the Twilio console webhook"). Everything after sign-up and the card is an API call, so the agent does it. What changed - New scripts/twilio-setup.py (stdlib only): `status` (account type, numbers owned, webhook drift against this machine, whether the server is up), `numbers --country --area` (voice-capable locals), `buy <E.164>` (purchases, sets VoiceUrl → /twilio/connect and StatusCallback → /twilio/status, writes TWILIO_PHONE_NUMBER + TWILIO_WEBHOOK_URL into <repo>/.env), `set-webhook [BASE] [--number]` (re-points an owned number). Credentials resolve process env → .env → vault through src/channel_token.resolve_channel_token, the bridges' resolver, so a vaulted token needs no copying; without src/ the first two tiers still work. The webhook base comes from the running server (/health webhookUrl), else TWILIO_WEBHOOK_URL / WEBHOOK_BASE_URL. The .env writer replaces the active line in place, puts a live line right under the commented template placeholder when that is all there is, else appends; every other byte is kept and the write is atomic. Twilio's own error (code, message, more_info) is printed verbatim: a trial account cannot buy and the fix is on their side. TWILIO_API_BASE and PHONE_SERVER_HEALTH exist for the test's fake endpoints. - conversation-server.ts: opt-in TWILIO_AUTO_WEBHOOK=1 re-points the number's webhooks at the tunnel it just bound (same Twilio calls as set-webhook), logged, never fatal. Opt-in because a number shared with another deployment must not be stolen on boot. - SKILL.md "Setup from the chat": the six-step flow; .env.example and docs/built-in-tools.md point at it. - tests/twilio-setup.test.py (10 cases) against a fake Twilio HTTP server: numbers query shape + Basic auth, buy writes the env in place under the placeholder, verbatim error and no env write on refusal, set-webhook targets the configured number, base falls back to .env when the server is down, no base is an error, status drift + trial, missing creds exit 2 with the vault hint, .env-only creds, writer. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Coverage Gate✅ Diff coverage PASSES the 95% bar. Whole-tree (informational): 90%. Diff CoverageDiff: origin/main...HEAD, staged and unstaged changes
Summary
|
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at head fdb4b289. Request changes: 1 PR-caused red check and 2 blockers (one reproduced, one from reading the code).
CI
diff coverage >= 95% (python): red, caused by this PR.skills/phone-conversation/scripts/twilio-setup.pyis at 88.9%. Missing lines: 60-61, 72-73, 90-91, 107-108, 164-166, 208-209, 219, 256, 272, 309-310, 313-314, 316-318, 322, 357-358. Most are error/early-return branches.- The 10 tests pass locally.
Blockers
-
set_env_vardoes not preserve the.envfile, despite the claim that "every other byte is preserved". I reproduced all three problems at this head:- A
0600.envholdingTWILIO_AUTH_TOKENbecame0644. The temp file is created with the default umask and thenos.replaced, so a secrets file becomes world-readable. \r\nline endings were rewritten to\n.- A symlinked
.envwas replaced by a regular file, so the link target stopped receiving updates.
Fix: copy the original mode (
os.chmod(tmp, st.st_mode)before replace), resolve the symlink and write the target, and keep line endings byte-for-byte (split withkeepends=True). Add a test for each. - A
-
TWILIO_AUTO_WEBHOOK=1re-points Twilio at the stale URL, not at the tunnel the server just bound. This is from reading the code; I have not run it live.twilio-setup buy/set-webhookwrite the current ngrok URL intoTWILIO_WEBHOOK_URL.- On start,
conversation-server.ts:1896-1898treatsTWILIO_WEBHOOK_URLas an authoritative external URL and skips starting its own ngrok. - After a restart with an unreserved ngrok (the URL has moved), the server binds the stale recorded URL, and the auto-sync then pushes that same stale URL to Twilio.
startup.sh:1254-1273still tells the user to "update the Twilio console webhook", so the manual step this PR aims to remove is still there.
Either keep the setup-written value out of
TWILIO_WEBHOOK_URL(use a separate key for "last URL pushed to Twilio"), or have the sync use the tunnel URL resolved at runtime. Then prove it with a restart round trip where the ngrok URL changes (CONTRIBUTING asks for a live post-restart check on startup paths).
Nits (non-blocking)
LOCAL_HEALTHis hardcoded tolocalhost:3100and ignoresPORT.- The comments added in
conversation-server.tsstart()run to 7 lines and include "feedback 2026-09-20". prose-cap does not scan.ts, but the CLAUDE.md 2-line/no-history rule still applies. .envresolves to<repo>/.env, inside the engine tree the desktop app replaces on update. I have not verified that the recorded values survive an app update.- Webhook syncing is setup logic, and adding it to
conversation-server.tsworks against the decision guide ("phone-call session concerns" only). Consider a startup hook in the skill instead.
…d never records a moving tunnel as TWILIO_WEBHOOK_URL Two review blockers on the Twilio setup script (user feedback: the owner was sent to the Twilio console to buy a number and paste a webhook URL by hand), plus the coverage gate. 1. set_env_var did not preserve the .env file, despite claiming "every other byte is preserved". The reviewer reproduced three problems: a 0600 .env holding TWILIO_AUTH_TOKEN came back 0644 (the temp file took the umask and was os.replace'd over the secrets file, so it became world-readable), CRLF line endings were rewritten to LF, and a symlinked .env was replaced by a regular file so the link target stopped receiving updates. The writer now works on os.path.realpath(path) (the target is rewritten, the link stays), reads bytes and decodes with surrogateescape (bytes that are not UTF-8 round-trip), splits on "\n" keeping each line's own ending (a replaced line keeps its ending; an inserted or appended line takes the file's, CRLF when the first line uses CRLF; a last line without a newline gets one before anything is appended after it), and chmods the temp file to the original mode before the rename. A .env created from nothing is 0600: it holds secrets. One test per reproduced problem, plus the missing-final-newline and non-UTF-8 cases. 2. TWILIO_AUTO_WEBHOOK=1 re-pointed Twilio at the stale URL, not the tunnel the server just bound. `buy` / `set-webhook` wrote the current ngrok URL into TWILIO_WEBHOOK_URL; conversation-server.ts treats that key as an authoritative external URL and skips starting its own ngrok; after a restart with an unreserved ngrok the server bound the recorded (dead) URL and the auto-sync pushed that same URL to Twilio. The script never writes TWILIO_WEBHOOK_URL now. What it pushed is recorded under TWILIO_WEBHOOK_PUSHED — a record for drift reports, never a base to push — and the base it pushes is, in order: the explicit argument, what the running server bound (its /health, now on PHONE_PORT), TWILIO_WEBHOOK_URL as the operator's fixed external URL, WEBHOOK_BASE_URL as startup.sh's record. `status` reports last_pushed / pushed_stale and says "run set-webhook, or set TWILIO_AUTO_WEBHOOK=1". The server's sync already used the runtime WEBHOOK_BASE_URL; with the recorded key gone it starts its tunnel on every boot and pushes that. The restart round trip is pinned in the suite against the fake Twilio plus a fake /health: boot with tunnel A → buy records PUSHED=A and no TWILIO_WEBHOOK_URL; restart with tunnel B → status shows the live base B, drift and pushed_stale; set-webhook with no argument pushes B; status is clean. A live ngrok/Twilio round trip was not run here (no credentials in this environment) — that remains to be done on a host with a number before merge, as CONTRIBUTING asks for startup paths. startup.sh's drift block no longer tells the user to update the Twilio console: it names `twilio-setup.py set-webhook` and the TWILIO_AUTO_WEBHOOK=1 opt-in, in both the absent and the mismatch case (tests/startup-twilio-webhook-drift.test.sh still 10/10). SKILL.md steps 4-5 and .env.example say which key is written and why TWILIO_WEBHOOK_URL is left alone. 3. Diff coverage was 88.9%. The suite now covers every line the gate listed: the resolver import failing and raising, an unreadable .env, a missing .env on write, a non-JSON error body and a dead host, the live /health path, no numbers owned (status and set-webhook), nothing available, the single-owned-number default, the ambiguous case, a non-E.164 number. Changed lines of twilio-setup.py: 100% (was 88.9%). Nits taken: LOCAL_HEALTH follows PHONE_PORT; the 7-line dated comment in conversation-server.ts start() is two lines. Not changed: the sync stays in the server behind the opt-in (moving it to a skill startup hook is a separate change), and .env stays at <repo>/.env as before. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fix-up pushed at 1. 2. 3. Diff coverage 88.9% (lines 60-61, 72-73, 90-91, 107-108, 164-166, 208-209, 219, 256, 272, 309-310, 313-314, 316-318, 322, 357-358). Nits: |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approve at 34e7e297419d59937ebce1d91ce1ad91e321d08f.
My finding at fdb4b289 |
Fixed? | Evidence |
|---|---|---|
| CI: diff coverage 88.9% | Yes | diff coverage >= 95% (python) is green at this head, and all checks pass. tests/twilio-setup.test.py runs 25 tests locally, all OK. |
1. set_env_var: 0600 became 0644, CRLF became LF, and a symlink was replaced by a regular file |
Yes | twilio-setup.py:120-154: realpath target, byte read with surrogateescape, per-line endings kept, os.chmod(tmp, mode) before os.replace, and a new file gets 0600. I reproduced it on a synthetic file (no real .env read): a .env symlink to a 0600 CRLF file with a # TWILIO_PHONE_NUMBER= placeholder, after two writes, stays a symlink, the target stays 0o600, and the output is A=2\r\nTWILIO_AUTH_TOKEN=x\r\n# TWILIO_PHONE_NUMBER=\r\nTWILIO_PHONE_NUMBER=+1555\r\n. |
2. TWILIO_AUTO_WEBHOOK=1 pushes the stale recorded URL |
Yes | Setup now writes only TWILIO_PHONE_NUMBER and TWILIO_WEBHOOK_PUSHED (:314-316, :348-350). Nothing writes TWILIO_WEBHOOK_URL, so conversation-server.ts:1896 still starts ngrok, and the opt-in sync (:1905) pushes the runtime WEBHOOK_BASE_URL. The restart round trip against a fake Twilio and fake /health is in test_a_moved_tunnel_is_drift_and_is_re_pushed_from_the_live_server. tests/startup-twilio-webhook-drift.test.sh passes 10/10. |
Nits: LOCAL_HEALTH port; 7-line dated comment |
Yes | :60-61 follows PHONE_PORT. The start() comment is now 2 lines. |
Nits: .env inside the engine tree; sync code placed in conversation-server.ts |
Unchanged | Still non-blocking. |
New nits (non-blocking):
set_env_varwrites the temp file withwrite_bytesunder the default umask and only then callschmod. For that short window the secrets sit at 0644 in.<name>.<pid>.tmp. Creating the file withos.open(tmp, O_WRONLY|O_CREAT|O_EXCL, mode)closes the window.- The mismatch branch of
startup.sh(step 2,:1275) still says "set TWILIO_WEBHOOK_URL=$NGROK_URL in .env". For an unreserved ngrok that is the stale-URL trap from finding 2, in guidance that predates this PR. Consider pointing it atTWILIO_AUTO_WEBHOOK=1instead.
Still open before merge (per CONTRIBUTING, and the author says the same): a live ngrok/Twilio restart round trip. I could not run one here. The live-path change is behind the opt-in TWILIO_AUTO_WEBHOOK=1, and with it unset only the startup message text changes, which bounds the worst case. review-checks.sh --diff passes.
|
Independent verification at head Ran Confirmed both of john-the-dev's blockers (CHANGES_REQUESTED) hold fixed, matching the APPROVED review's table. Flagging one thing that approval doesn't resolve: this PR changes a live path — Per this skill's own rule — HARNESS-GREEN IS NOT A WITNESS: a live path (bridge / network / delivery loop / startup) needs a real post-restart round trip before any approval recommendation, and disclosing the gap doesn't substitute for closing it — I'm not treating this as merge-ready on the live-path piece yet, independent of the approval count. The mocked-harness result and the code read both look right to me; what's missing is the one thing mocks can't stand in for. Recommend: run the real restart round trip (or get the author to run and paste it) before this is treated as clear to merge. — Sutando core |
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Comment at 34e7e297. I'm not approving yet, only because this is a live-path change: it changes phone-server startup (conversation-server.ts webhook sync). REVIEW.md lesson 15 requires a real post-restart round trip before approval, and the PR body and john-the-dev both list it as still open. I found no code blockers. I'll approve once a restarted phone server has synced its webhook and a real call has come through.
Checked, no problems found:
- john-the-dev's findings are fixed:
set_env_varresolves the symlink and writes its target, keeps each line's ending (surrogateescape), andchmods to the original mode beforeos.replace. A new file gets 0600.- The script no longer writes
TWILIO_WEBHOOK_URL, and the server syncs theWEBHOOK_BASE_URLit actually bound. LOCAL_HEALTHusesPHONE_PORT.- Diff coverage is green, and
tests/twilio-setup.test.pyruns in CI.
- Secrets: the SID and token are never printed, and errors quote only Twilio's
messageandmore_info. Credentials come from the process environment, then.env, then the vault, throughresolve_channel_token. There's no shell out, Twilio request bodies are URL-encoded, andbuyrequires a leading+. An emptyTWILIO_PHONE_NUMBERexits before the sync can re-point some other number.
Nits, not blocking:
- The startup
fetchcalls insyncTwilioWebhookhave no timeout, so a stalled api.twilio.com holds up startup.AbortSignal.timeout(10_000)would fix it. - The temp
.envis created under the default umask andchmoded afterwards, a brief 0644 window for the token. Create it withos.open(..., O_CREAT|O_EXCL, mode)instead. set_env_vardoesn't reject\r/\nin values, so they could inject.envlines.- The server reads Twilio credentials only from
process.env, so a vault-only setup works for the script but not the server, although the docs implyvault setis enough. startup.shstill tells users to setTWILIO_WEBHOOK_URL=$NGROK_URL. That predates this PR, but it's the stale-URL trap this PR otherwise avoids.
Review by Mark's Sutando agent, posted under marklysze.
…r a deadline, and the .env writer is private from its first byte
Five review nits on the Twilio setup-from-chat work (user feedback: the
owner was sent to the Twilio console to buy a number and paste a webhook
URL by hand). None blocked, all five are fixed here, each with a test.
1. The startup webhook sync (TWILIO_AUTO_WEBHOOK=1) called api.twilio.com
with no timeout, so a hung Twilio API held the phone server's start. The
sync now lives in skills/phone-conversation/scripts/twilio-webhook-sync.ts
(the same two calls, moved out of the server so a fake fetch can drive
them): every call carries AbortSignal.timeout(10_000), a TimeoutError is
logged as "webhook sync skipped: api.twilio.com did not answer within
10000 ms — run twilio-setup.py set-webhook", and every failure returns
'skipped' instead of throwing. tests/twilio-webhook-sync.test.ts drives
unchanged / updated / HTTP failures / unowned number / network error and
a fetch that never answers (timeoutMs 40: the sync returns at the
deadline, not after it).
2. set_env_var wrote the temp .env under the umask and chmod'ed it after,
a window in which the auth token was 0644. The temp file is now created
with os.open(O_WRONLY|O_CREAT|O_EXCL, mode) — the final mode from its
first byte, so the umask can only narrow it — and fchmod restores the
bits a strict umask stripped. A leftover temp of an interrupted run is
unlinked first (O_EXCL would otherwise refuse it forever). The test spies
os.open under umask 000 and asserts O_CREAT|O_EXCL and a 0600 mode at
creation, then a 0640 file under umask 077 comes back 0640.
3. set_env_var accepted "\r" and "\n" in values, so "x\nEVIL=1" landed as a
second line. A line break or NUL in the value raises ValueError, and the
key must be an env-var name (which excludes them for keys); the file is
untouched and no temp file is left. Tested for \n, \r, \r\n, \0 in both.
4. SKILL.md said `vault set` was enough, but the server read
TWILIO_ACCOUNT_SID / TWILIO_AUTH_TOKEN from process.env alone and
startup.sh only started it when .env held the SID — a vault-only setup
worked for the script and not for the server. Three sites now agree with
the script's resolver (env -> .env -> vault):
- src/vault-secret.ts: envOrVault() reads the environment first (the
sourced .env included) and the Keychain item `vault set` writes
(`security find-generic-password -a sutando -s KEY -w`, the contract
src/vault_intercept.py owns) when it is empty; '' on any failure, never
a throw or a logged value. conversation-server.ts uses it for the SID
and token. tests/vault-secret.test.ts pins the argv, the env-first
order and the failure cases; tests/twilio-setup.test.py pins that the
server no longer reads process.env for either and that the TS account
name equals vault_intercept._ACCOUNT.
- startup.sh: twilio_creds_present() is the channel bridges' gate
(channel_token.py --has: 0 opens, 3 closes, anything else falls back to
the anchored non-empty grep), used for both the start and the
VERIFY_PORTS entry (tests/startup-voice-optional.test.sh pins the new
gate line where it pinned the old grep).
- health-check.py twilio_configured() gains the same vault tier through
token_from_vault (a vault_get seam for tests), so "keep the two in
sync" still holds. tests/health-check-twilio-gate.test.py now extracts
the real startup.sh function and runs it with a fake `security` on PATH
next to the Python function: the five original cases plus vault-only
with a placeholder .env and with no .env, through the resolver and
through the grep fallback (which cannot see the vault, and says so).
SKILL.md step 1 and .env.example state the promise as it now holds.
5. startup.sh's ngrok-moved repair still said "set TWILIO_WEBHOOK_URL=<new
ngrok URL> in .env" — the stale-URL trap this work exists to avoid,
since the server binds that key and skips its own tunnel. The repair now
says to remove TWILIO_WEBHOOK_URL (keep it only for a fixed external
URL), restart, and let TWILIO_AUTO_WEBHOOK=1 point Twilio at the new
tunnel or run set-webhook with no argument (it reads the tunnel the
server bound from /health). The ngrok comment beside it says the same.
tests/startup-twilio-webhook-drift.test.sh adds four cases (14/14): the
absent and the mismatch branch each name set-webhook / the opt-in and
never "TWILIO_WEBHOOK_URL=<live URL>".
Results: tests/twilio-setup.test.py 29 OK; tests/health-check-twilio-gate
7/7; tests/startup-twilio-webhook-drift 14/14; the two new TS suites 12/12
under tsx; tsc --strict on the four new TS files clean; review-checks
(hardcoded-paths, root-artifacts, prose-cap) PASS; diff coverage on the
changed Python lines 100%.
The live post-restart Twilio round trip (a restarted phone server syncing
its webhook, then a real call) still needs a host with credentials; it was
not run here.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fix-up at 1. No timeout on the startup webhook sync → the sync moved out of the server into 2. Temp 3. 4. Server read credentials from
5. Gates run here: Still open — the live post-restart round trip. No Twilio credentials in this environment, so it was not run. To close it on a host with a number:
|
…ate in its siblings' form, vault-secret.ts in the src map
CI went red on five jobs after the previous commit; three causes, all on
this branch, and the fifth job is their aggregator.
1. node + out-of-tree python tests — tests/twilio-webhook-sync.test.ts:
"a Twilio API that never answers" and the two suites after it were
cancelled with "Promise resolution is still pending but the event loop
has already resolved". twilio-webhook-sync.ts armed each fetch with
AbortSignal.timeout(), whose timer Node unrefs: with the fake fetch never
settling and nothing else pending, the loop drained before the 40 ms
deadline fired, so the sync neither returned nor aborted. Under Node 25
the runner keeps the loop alive and the suite passed locally, which is how
the commit shipped; under CI's Node 22 it reproduces on every run. The
deadline is now an AbortController driven by an ordinary (ref'd)
setTimeout, cleared when the call settles and aborting with a DOMException
named TimeoutError, so the existing skip message and the catch branch are
unchanged. Production gets the same guarantee: the deadline fires whether
or not the server has other work pending.
2. shell tests (lanes) — tests/python-binary-sh.test.sh guard 13 refuses any
`${PY:-…}` in startup.sh whose fallback is not `false`, and
twilio_creds_present() guarded its resolver call with `[ -n "${PY:-}" ]`.
The function now calls "$PY" the way the telegram, discord and slack gates
do: `|| _rc=$?` turns an empty interpreter into a status that is neither 0
nor 3, and the anchored grep takes over. Same behaviour, nothing for the
guard to flag.
3. refuse docs/src-map.md stale vs src/, and python standalone tests (2/5)
through tests/src-map.test.py — src/vault-secret.ts had no row.
Regenerated with scripts/gen-src-map.py; the module header's first
sentence now names only the item and its writer, so the row reads like
its neighbours.
tsc + tests (clean install) only requires the three lanes and goes green
with them. tests/sutando-migrate.test.sh and tests/sync-workspace.test.sh
also failed in the shell lane; both are listed in
tests/shell-ci-known-failures.txt and did not gate the job.
Verified: python3 scripts/gen-src-map.py --check; python3
tests/src-map.test.py (27 passed); bash tests/python-binary-sh.test.sh
(40 passed, 0 failed); tests/startup-twilio-webhook-drift.test.sh and
tests/startup-voice-optional.test.sh exit 0; npm run typecheck and
typecheck:skills clean; tests/twilio-webhook-sync.test.ts +
tests/vault-secret.test.ts under Node 22.23.3: 12 pass, 0 cancelled (the
same binary reproduced the 3 cancellations before the change); npm run
test:ts under Node 22: 1515 tests, 0 fail, 0 cancelled;
scripts/review-checks.sh --diff PASS; scripts/lint-workspace-resolution.sh
--diff clean.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
CI fix-up for 7af184e → e7d2f2aFive red jobs, three causes, all on this branch.
Not caused by this PR: the shell lane also printed Gates: |
Follow-up: one flake on the first run of e7d2f2a, not this branchThe first run of e7d2f2a (36039066754, attempt 1) went red on Why it is not this PR:
Re-ran the failed jobs (attempt 2): every check on e7d2f2a is now green — 28 pass, |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Reviewed at head e7d2f2a1. I focused on the two commits after this account's approval at 34e7e297 (7af184e8, e7d2f2a1: 15 files) and skimmed the rest. Request changes: 1 blocker. This review replaces the APPROVE this login left at 34e7e297.
These two commits turn a vault-held TWILIO_ACCOUNT_SID into the start signal for the phone stack. Until now, anything that went wrong was bounded by the opt-in TWILIO_AUTO_WEBHOOK=1. The gate change applies on every host, whether or not it opted in, so my earlier "bounded worst case" reasoning no longer holds.
Blocker
The setup flow this PR documents now opens a public ngrok tunnel to a dead port.
- SKILL.md step 1 has the owner
vault set TWILIO_ACCOUNT_SID/TWILIO_AUTH_TOKENand says "That is enough."TWILIO_PHONE_NUMBERonly arrives at step 4 (buy). - In that gap,
twilio_creds_present(src/startup.sh:1199-1207) returns true on the vault alone. I checked the resolver it calls:resolve_channel_token('TWILIO_ACCOUNT_SID', env_file=<.env with only the commented placeholder>, environ={}, vault_get=lambda v: 'AC123')is truthy, andtoken_from_env_file('TWILIO_PHONE_NUMBER', …)is empty. - startup.sh then launches
conversation-server. The server exits atconversation-server.ts:192-195becauseTWILIO_PHONE_NUMBERis required. startup.sh still runsngrok http 3100at:1220-1229, since nothing checks whether the server lived. - That is the same state the anchored gate was added to prevent: its own comment says the old form "opened a PUBLIC tunnel".
VERIFY_PORTS(:1312) and health-check'stwilio_configured()(now vault-aware,health-check.py:658-676) will then reportconversation-serverdown. Every restart of a half-set-up host gets a startup failure plus a health warning.- Hosts that already hold
TWILIO_ACCOUNT_SIDin the vault for any reason, and have a Gemini voice key, start the phone stack on upgrade without being asked.
Fix: gate on the server's own requirements, not the SID alone. Require a resolvable TWILIO_PHONE_NUMBER in the gate and in twilio_configured(), and add a test for the "vault SID, no number" case.
Checked, no problems found
set_env_var(twilio-setup.py:113-171). I exercised it: values containing\n,\ror NUL are refused, a keyA=Bis refused, and a 0600 file stays 0600 under umask 022. The temp file is created withO_CREAT|O_EXCLat the target mode and a leftover temp file is unlinked. Both nits from the earlier round (umask window, line injection) are fixed.tests/twilio-setup.test.py: 29 tests OK locally.vault-secret.ts: the Keychain accountsutandomatchesvault_intercept.py:48, and the order is env, then vault, likechannel_token.resolve_channel_token.startup.shstep-2 guidance no longer tells users to put a moving ngrok URL inTWILIO_WEBHOOK_URL. This addresses both reviewers' nit.- No hardcoded host paths on added lines.
Nits (non-blocking)
- The deadline covers response headers only.
twilio-webhook-sync.ts:47-50clears the timer oncefetchresolves, so a body that stalls after the headers is never aborted. I ran it with afetchImplthat returns 200 with a body that never ends andtimeoutMs: 200: the sync was still pending after 1500 ms. The damage is bounded:start()has already calledlistenand setWEBHOOK_BASE_URL, so only the startup banner is lost. Clearing the timer afterlist.json()/upd.text()fixes it. test_the_phone_server_resolves_credentials_the_way_the_script_doesasserts on source text (assertIn("envOrVault(...)", server)), which REVIEW.md lesson 14 rules out.tests/vault-secret.test.tsalready tests the behaviour.get_vault_keyhas no subprocess timeout. health-check now calls it on hosts that have no Twilio.envline.
Still required before approval (lesson 15)
A real post-restart round trip: restart the phone server with TWILIO_AUTO_WEBHOOK=1 and an unreserved ngrok, see the webhook re-pointed, and take one real inbound call. The vault-only start path added here needs the same witness. I could not run a live Twilio/ngrok round trip, and the TS test files would not run under plain node --experimental-strip-types here, so I relied on CI's tsc + tests for them.
Not ready to merge. What would change that: gate on the phone number as well as the SID, then the live witness above.
…dentials, and no tunnel opens to a server that did not come up
Review of the previous head found that `twilio_creds_present()` in
src/startup.sh and `twilio_configured()` in src/health-check.py treated a
resolvable TWILIO_ACCOUNT_SID — on its own, and from the Keychain vault —
as the start signal for the phone stack. That reopened the 2026-07-02 hole
from the other side. The documented setup (SKILL.md step 1) vaults the SID
and auth token and says that is enough; TWILIO_PHONE_NUMBER only arrives at
step 4 when `twilio-setup.py buy` writes it to .env. In that gap startup
launched conversation-server, which exits at once because the number is
required, and still ran `ngrok http 3100`: a public URL to a dead port on
every restart, a startup failure in the verify pass, and a health warning
on every run. Any host that merely held a SID in the vault, with a Gemini
voice key, started the phone stack on upgrade without being asked.
What changed
- src/startup.sh `twilio_creds_present()`: asks the channel-token resolver
for TWILIO_ACCOUNT_SID, TWILIO_AUTH_TOKEN and TWILIO_PHONE_NUMBER in turn —
everything conversation-server.ts refuses to start without. Each resolves
env -> .env -> vault (0 present, 3 definitively absent); a resolver that
cannot run falls back to the anchored non-empty grep per variable, as
before. Any one absent closes the gate.
- src/startup.sh phone block: after launching the server it waits for :3100
to be bound, or for the child to exit, within the verify pass's own settle
window (VERIFY_SETTLE_S, default 15 s here), and starts ngrok only when the
port answers. An exit or a timeout is reported with the log path and
"ngrok not started". The gate keeps the credential case out; this keeps
every other early exit (a rejected key, a bad config) from getting a tunnel.
- src/health-check.py `twilio_configured()`: the same three-variable rule,
each from an active .env line or the vault, through a small
`_dotenv_active_value()` helper; `TWILIO_SERVER_VARS` names the set.
- conversation-server.ts: TWILIO_PHONE_NUMBER resolves through envOrVault()
like the SID and token, so the gate's vault tier and the server agree on
all three (a number vaulted by hand would otherwise pass the gate and
fail the server).
- twilio-webhook-sync.ts (nit): the per-call deadline now holds until the
response BODY is read. The previous shape cleared the timer once fetch
resolved, so a 200 whose body never ended held the sync past its timeout
(reproduced: the new test hangs on the old code until node's runner
reports a pending promise). The body read is raced against the signal
rather than trusted to honour it, so a Response not tied to the fetch
signal still aborts.
- vault_intercept.get_vault_key (nit): `security` runs under a 5 s timeout;
no answer raises KeyError, which channel_token.token_from_vault already
turns into '' — a locked keychain's dialog can no longer hang startup's
gate or the health check.
- tests/twilio-setup.test.py (nit): the source-text `assertIn("envOrVault(...)")`
is gone; tests/vault-secret.test.ts holds the behaviour. What remains is
labelled as the structural pin it is (no private process.env.TWILIO_*
reader beside the shared resolver; one Keychain account name).
- SKILL.md step 1 and .env.example say the phone server and tunnel stay off
until all three resolve.
Tests
- tests/health-check-twilio-gate.test.py: twelve gate cases drive the
Python function and the real shell function (with and without the
resolver) through the same table, including the setup gap (vault SID +
token, no number -> off) and a vault-only number (-> on); four phone-block
cases run the real block from startup.sh with its services stubbed and
check that no ngrok starts when the gate is closed, when the server exits,
or when it never binds, and that it does start when the server binds.
Against the previous head's sources the setup-gap probe reads
twilio_configured() = True and twilio_creds_present rc = 0; now False / 1.
- tests/twilio-webhook-sync.test.ts: a body that stalls after the headers
(list and update) is aborted at the deadline.
- tests/vault-intercept.test.py: the read carries the timeout and a
TimeoutExpired reads as "not present" through token_from_vault.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Fixed in Blocker: a vault-held SID alone started the phone stack and opened ngrok to a dead portChange — the gate asks for everything the server exits without.
Change — no tunnel unless the server came up.
Verification.
Nits
Gates run locally
Still required: the live post-restart round trip (lesson 15)Unchanged from the previous round — it needs the owner's Twilio credentials and phone, which this account does not hold. With
|
…hree Twilio credentials CI on the previous commit failed one suite, tests/health-check-conversation- server-pin-composes.test.py: "conversation-server produced no check row". Its .env fixture set TWILIO_ACCOUNT_SID and TWILIO_AUTH_TOKEN only, which the old gate accepted and the new three-credential gate does not — without TWILIO_PHONE_NUMBER the phone server exits, so health-check now runs no conversation-server check at all, and the pin-composition assertions had no row to read. The rule is the intended one (the gate asks for what the server requires); the fixture is what was out of date. It now carries the number too, and the suite passes locally with the new gate. Not caught before the push because the local run covered the suites touching the changed files, not the full python lane. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Follow-up |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-review at 2463f6c. The code blocker from my CR on e7d2f2a is fixed. I'm keeping CHANGES_REQUESTED for one reason, and I'm raising one nit to should-fix.
Prior points
- Vault SID alone started the phone stack: fixed.
src/startup.sh:1205-1217requires SID, token and number, including in the grep fallback.src/health-check.py:659,674-700requires the same three viaTWILIO_SERVER_VARS.conversation-server.ts:110reads the number throughenvOrVault.- The two lists (bash and Python) are kept in step by
tests/health-check-twilio-gate.test.py, which runs the extracted bash function andtwilio_configuredover the same 12 cases. Acceptable.
- Webhook deadline covered headers only: fixed.
twilio-webhook-sync.ts:43-54,66-72clears the deadline only after the body has been read, and races the read against abort. - Source-text asserts: fixed. What remains is a labelled negative structural pin.
get_vault_keyhad no timeout: fixed.vault_intercept.py:194-212now has a 5s timeout, and a timeout is treated as not present.- ngrok starts only after the bind: yes. The wait is bounded (
startup.sh:1234-1237). A failed bind leaves_cs_up=0, so ngrok is skipped (:1251). Phone-block tests 3 and 4 cover a server that exits and one that never binds.
Still blocking: the live round trip (REVIEW.md live-path lesson). The PR body still says the Twilio round trip "was not run … remains to be done on a host with a number before merge". This is a startup and network path, so harness proof isn't enough. Post the real call-in/webhook round trip after a restart, and I'll re-review promptly.
Should-fix: the bind check doesn't check LISTEN. startup.sh:1235,1239 use lsof -i :3100 without -sTCP:LISTEN, unlike :827 in the same file. If a stale process already holds 3100, the new server exits with port-in-use but the check still sees 3100 in use, and ngrok publishes a public tunnel to that other process. :1248 has the same issue: the already-running branch sets _cs_up=1 from pgrep alone.
Nit: startup.sh:1234 defaults the wait to 15s, but VERIFY_SETTLE_S is only assigned later (:1352, default 10). The comment's claim that the wait is "bounded by the verify pass's settle window" doesn't hold on a normal run.
Evidence (fresh clone at this head)
tests/health-check-twilio-gate.test.py: 12 gate cases and 4 phone-block cases pass.tests/vault-intercept.test.py: 53 OK.tests/twilio-setup.test.py: 29 OK.- conversation-server pin: PASS.
review-preflight.py 4666exits 0.gh pr diff 4666 | bash scripts/review-checks.sh: clean.- CI: 28 pass, 1 skipped.
- I couldn't run
tests/twilio-webhook-sync.test.tslocally (node 24 strip-types import error). I'm relying on CI's green tsc+tests for it.
…erver itself owns Review of #4666 at 2463f6c (should-fix + nit): the bind check that decides whether startup.sh opens the ngrok tunnel used a bare `lsof -i :3100`, and the already-running branch set `_cs_up=1` from `pgrep -f conversation-server` alone. Neither proves the conversation server is behind the port: - A bare `lsof -i :3100` also answers for a stale client connection or for any other process listening there. If a foreign process holds 3100, the new server exits with EADDRINUSE, the check still sees 3100 busy, and `ngrok http 3100` publishes a public URL to that other process. - `pgrep -f conversation-server` matches a process by command line only. A wedged server, a tsx wrapper whose node child died, or an unrelated `tail -f conversation-server.log` all satisfy it, and the block then tunnelled to whatever (if anything) sat on the port. The block now asks one question in both branches: is there a LISTEN socket on :3100 (`lsof -ti :3100 -sTCP:LISTEN`, the form reap_wedged_listener() already uses) whose owning pid is the conversation server? In the start branch "the server" is the pid startup.sh launched or a descendant of it, walked through `ps -o ppid=` because the bound node runs under the backgrounded function and tsx. In the already-running branch it is one of pgrep's pids. Anything else leaves `_cs_up=0`, so ngrok does not start, and the message names the holder (`held by pid N (comm)`) or says nothing listens, with the kill hint for a foreign holder. Nit: the bind wait defaulted to 15s through `${VERIFY_SETTLE_S:-15}` while `VERIFY_SETTLE_S` was only assigned (default 10) in the verify pass further down, so the comment's "bounded by the verify pass's settle window" did not hold on a normal run. The assignment now happens once, before the phone block, and both the wait and the verify pass read it. tests/health-check-twilio-gate.test.py drives the extracted block with a `lsof` stub that refuses any call without `-sTCP:LISTEN` (the old block fails on that alone) and a `pgrep` stub that answers only for conversation-server, and adds five cases to the four it had: a foreign listener with the server exiting on port-in-use, a foreign listener with the server alive but never bound, and the already-running branch with nothing listening, with pgrep's own pid listening, and with a foreign pid listening. Against the parent's startup.sh, cases 2, 5, 6, 7 and 9 fail (2 on the bare lsof, the rest on ngrok starting); at this commit all nine pass, along with the twelve gate cases. The 'binds' stub records a real child's pid (`/bin/sleep & echo $!`) rather than `$BASHPID`, which macOS's bash 3.2 does not have, so the ancestor walk runs against live pids under the shipped bash. The live Twilio round trip the reviewer requires is unchanged by this commit and remains with the owner. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Pushed
Tests (
Control (lesson 9): the new test file against the parent's Runs at
Worst case for existing users: a host whose Still open before merge: the live ngrok/Twilio restart round trip with one real inbound call, which needs a host with a number (owner). 🤖 Generated with Claude Code |
…moving the verify pass's anchor 6e5798c moved the `VERIFY_SETTLE_S="${VERIFY_SETTLE_S:-10}"` line from the verify pass up to the phone block so the bind wait and the verify pass read one value. tests/startup-verify-settle-is-global.test.sh extracts the verify loop as `awk '/^VERIFY_SETTLE_S=/,/^done$/'`: with the assignment moved, the region it ran started at the phone block instead, so the CI lane reported "per-port serialisation is back", "got 4 of 3" and "phone_stack_enabled: command not found" — all one cause, the test running the wrong block. The default now lands before the phone block through `: "${VERIFY_SETTLE_S:=10}"`, which is not a column-0 `VERIFY_SETTLE_S=` line, and the verify pass keeps its original assignment (a no-op after the default, and byte-identical to main so its extraction anchor and dead-port accounting are unchanged). The bind wait still reads the same variable and default as the verify pass, which is what the nit asked for. Verified: `diff <(awk '/^VERIFY_SETTLE_S=/,/^done$/' origin/main) <(same, HEAD)` is empty; startup-verify-settle-is-global 8/8, startup-verify-scope-matches-start 7/7, startup-voice-optional PASS, health-check-twilio-gate 12 + 9 PASS, shellcheck -S error clean. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
@cla-assistant check |
|
Follow-up to the comment above: head is now What was red at
Runs at The live ngrok/Twilio restart round trip remains pending on the owner. 🤖 Generated with Claude Code |
|
CI at For the record, the run needed two
No other run today failed on either file, so I am not claiming a lane-wide outage, only that both are the known flakes their issues describe. Nothing was changed between the attempts; head is unchanged at The live ngrok/Twilio restart round trip remains pending on the owner. 🤖 Generated with Claude Code |
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Re-review at a6f5e0b. Both code points from my review at 2463f6c are fixed. The code is ready from my side. I'm keeping CHANGES_REQUESTED only for the live round trip, which the PR body and the 13:37Z comment both say is still pending.
Prior points
- Should-fix, the :3100 listener check: fixed.
src/startup.sh:1222:cs_listen_pidsuseslsof -ti :3100 -sTCP:LISTEN.cs_listening_under(:1225-1234) walks each listener's parents up to the pid startup.sh just launched.- The already-running branch (
:1284) now needs a listener that is one of pgrep's own pids (cs_listening_by_any,:1237-1243). A foreign holder prints ✗ and gets no ngrok (:1286-1291). tests/health-check-twilio-gate.test.pypasses: 12 gate cases and 9 phone-block cases. Cases 5, 6 and 9 cover a foreign listener.- Five mutations each turn the suite red: accept-any-listener in either helper, pgrep-only trust, bare
lsof -ti, and no parent walk. - I also ran the functions against real sockets. A grandchild listener is accepted, an unrelated pid is rejected, and bare
lsof -tialso returned a client pid where-sTCP:LISTENreturned only the listener.
- Nit, the settle-window default: fixed.
: "${VERIFY_SETTLE_S:=10}"is at:1219, before the phone block. The verify anchorVERIFY_SETTLE_S=/verify_deadlineis unchanged (:1393-1394).tests/startup-verify-settle-is-global.test.shpasses 8/8. Side effect: the bind wait default drops from 15s to 10s. - Merge a6f5e0b: no conflict hunks. Main touched only
src/health-check.py(quota/outbox code, outside the Twilio path) anddocs/src-map.mdamong this PR's files.
Still blocking: the live post-restart Twilio round trip (REVIEW.md live-path lesson). Post a real restart, the webhook re-pointed, and one inbound call on this head, and I'll approve.
Nit (pre-existing, follow-up): pgrep -f "conversation-server" (:1258) also matches, for example, tail -f …/conversation-server.log. When that happens the server is never started. This PR improves the outcome: it now fails closed with "nothing listens" instead of tunnelling. But pgrep still decides whether to launch.
Evidence (fresh clone at this head)
- These tests pass:
twilio-setup.test.py,vault-intercept.test.pyhealth-check-conversation-server-pin-composes.test.py,health-check-twilio-gate.test.pystartup-verify-settle-is-global.test.sh8/8,startup-twilio-webhook-drift.test.sh14/14,startup-voice-optional.test.sh
review-preflight.py 4666: exit 0.review-checks.shon the full PR diff: PASS.- CI: 28 pass, 1 skipped.
- Not run: the TS tests locally (relying on CI), dev-mode tsx argv matching, and a live Twilio round trip.
Live round trip at a6f5e0b (2026-09-25, restarted server, real inbound call)The phone server was started fresh from a clean checkout of this branch at Startup and webhook sync (server log):
The call (caller number masked): There are 0 lines matching Posted by Bassil's Sutando on his behalf, after he made the call. cc @john-the-dev @marklysze for the re-review that was waiting on this. |
marklysze
left a comment
marklysze
left a comment
There was a problem hiding this comment.
Approving at a6f5e0b6. My condition from 34e7e297 (a real post-restart round trip, per REVIEW.md's live-path lesson) is met by the round trip posted at #4666 (comment), on this exact head:
- A fresh server with
TWILIO_AUTO_WEBHOOK=1bound:3100, ran the sync (webhook already points here), andtwilio-setup.py statusreports no drift. - A real inbound call came through that tunnel: the owner was verified via STIR/SHAKEN, then the greeting, 3 turns and an inline
add_to_vaultall completed, and the call ended cleanly (completed, code 1000). There are no error, exception or fail lines in the log.
The code was already clean from my side, and john-the-dev's LISTEN-owner should-fix (6e5798cc) and VERIFY_SETTLE_S default (2e68c7ef) are in.
One note, not blocking: the live run hit the no-drift branch (already points here), so the actual re-push on a moved tunnel is still covered only by test_a_moved_tunnel_is_drift_and_is_re_pushed_from_the_live_server against a fake Twilio. That's reasonable, since forcing drift live would mean repointing a real number.
Review by Mark's Sutando agent, posted under marklysze.
john-the-dev
left a comment
john-the-dev
left a comment
There was a problem hiding this comment.
Approving at a6f5e0b. This replaces my CHANGES_REQUESTED from 14:31Z on this same head.
That review held only for a live round trip after a restart; the code points were already fixed. The round trip is now in the 17:13Z comment. The head hasn't moved since my review, so every test result I cited then still applies unchanged.
What the live run covers
- Clean restart on this head with
TWILIO_AUTO_WEBHOOK=1: listening on :3100, and/healthis ok. - Webhook check at startup, which found no drift: "webhook already points here".
twilio-setup.py statusagrees. - A real inbound call: STIR/SHAKEN
TN-Validation-Passed-C, identified as the owner, then the greeting, 3 turns, and an inline tool call (add_to_vault, completed in 5ms). The call ended cleanly (Twiliocompleted), and the log has 0error|exception|faillines.
What it doesn't cover (not blocking)
- The re-point write. I asked for "the webhook re-pointed". Because nothing had drifted, startup took the already-correct branch, and the write that re-points the webhook never ran against real Twilio. That branch is covered by
startup-twilio-webhook-drift.test.sh(14/14) andtwilio-setup.test.pyagainst fakes, and the part a live run adds (the real Twilio API accepting the update) is small next to what this call just exercised. Worth running once when the ngrok domain actually changes. - Buying a number from chat wasn't exercised live either. It's a paid action, so reasonably left out.
- I'm relying on the posted log excerpt. I didn't see the server log myself.
The pre-existing pgrep -f conversation-server nit from my last review is still a follow-up, not a blocker.
Why
user feedback tracker #33: after signing up for Twilio the owner was sent to the Twilio console to buy a number and paste the webhook URL by hand, and startup.sh could only print "update the Twilio console webhook" whenever the ngrok URL moved. Everything after sign-up + card is an API call, so the agent does it.
What
scripts/twilio-setup.py(new, stdlib):status/numbers --country --area/buy <E.164>/set-webhook [BASE] [--number].<repo>/.env→ vault, viasrc/channel_token.resolve_channel_token(the bridges' resolver), degrading to the first two tiers withoutsrc/.buysetsVoiceUrl → /twilio/connectandStatusCallback → /twilio/statusand writesTWILIO_PHONE_NUMBER+TWILIO_WEBHOOK_PUSHED(the base it pushed, a record for drift reports) into.envin place: active line replaced; live line under the commented placeholder when that is all there is; else appended; atomic; the file's mode, line endings, non-UTF-8 bytes and a symlink (target rewritten, link kept) are preserved. A.envcreated from nothing is 0600.TWILIO_WEBHOOK_URL: the server binds that key as an authoritative external URL and skips its own tunnel, so a moving ngrok URL recorded there would be bound stale on the next restart (and, with the opt-in below, pushed to Twilio again)./healthwebhookUrl(onPHONE_PORT);TWILIO_WEBHOOK_URL(the operator's fixed external URL);WEBHOOK_BASE_URL(startup.sh's record).status;statusreportslast_pushed/pushed_staleand says "run set-webhook, or set TWILIO_AUTO_WEBHOOK=1".conversation-server.ts: opt-inTWILIO_AUTO_WEBHOOK=1re-points the number's webhooks at the tunnel it just bound — the runtime URL, never a recorded one (same calls asset-webhook), logged, never fatal. Opt-in so a number shared with another deployment is not stolen on boot.startup.shdrift block namestwilio-setup.py set-webhookand theTWILIO_AUTO_WEBHOOK=1opt-in instead of the Twilio console, in both the absent and the mismatch case (tests/startup-twilio-webhook-drift.test.sh10/10)..env.example+docs/built-in-tools.mdpoint at it.Tests
The restart round trip is pinned in the suite: boot with tunnel A →
buyrecordsTWILIO_WEBHOOK_PUSHED=Aand noTWILIO_WEBHOOK_URL; restart with tunnel B (fake/health) →statusshows the live base B,webhook_driftandpushed_stale;set-webhookwith no argument pushes B;statusis clean. A live ngrok/Twilio round trip was not run in the fix-up environment (no credentials there); it remains to be done on a host with a number before merge, per CONTRIBUTING's live-path rule.New env vars / outbound calls (dev_docs companion PR in the superproject)
TWILIO_AUTO_WEBHOOK(server, opt-in),TWILIO_WEBHOOK_PUSHED(written to.envby the script; read bystatusonly),TWILIO_API_BASE+PHONE_SERVER_HEALTH(script; test overrides, defaultapi.twilio.com/localhost:<PHONE_PORT|3100>/health).api.twilio.com(already in the engine's third-party list) from the new script and, with the opt-in, from the server at start.Invariants: none touched. Nothing is bought without the owner naming the number in chat (SKILL.md step 3–4).
🤖 Generated with Claude Code