Conversation identity: key conversations by (platform, account_id, user_id) - #149
Open
nandanrao wants to merge 24 commits into
Open
Conversation identity: key conversations by (platform, account_id, user_id)#149nandanrao wants to merge 24 commits into
nandanrao wants to merge 24 commits into
Conversation
A conversation is (platform, account_id, user_id), not a user id. The log
tables could not express that.
25 messaging_accounts registry, PRIMARY KEY (platform, account_id), FK to
credentials with ON DELETE CASCADE -- without the cascade a messaging
credential becomes undeletable, and delete-then-recreate IS the account
reconnection path.
26 messages gains nullable account_id/platform plus an index on
(userid, account_id, timestamp). Deliberately NOT an ALTER PRIMARY KEY:
hsh is fnv64a over the whole content blob and the account is inside that
blob, so ON CONFLICT (hsh, userid) is already transitively account-scoped.
The rewrite the plan called for would have peaked past free space on two
nodes of a 384 GiB table.
27 chat_log and 28 responses DO need ALTER PRIMARY KEY -- their conflict
targets ARE their primary keys, so widening without the migration turns a
silent row drop into a 42P10 that log.Fatalf's the sink. Both carry the
four-step runbook in their headers; neither may be applied without the
matching scribble build.
Backfill scripts are separate because 1.8M and 106M rows exceed one implicit
transaction and CockroachDB v24.1 has no DO blocks.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
hermes stamps normalized top-level `account_id` and `platform` on all three event shapes; the Messenger derivation follows the echo-inversion rule (sender.id when message.is_echo, else recipient.id). That rule now exists in four languages, so it is bound to one shared fixture at testdata/event-envelope/ that Rust, JS, Go and SQL all load -- verified load-bearing by corrupting a vector and watching both suites fail on it. The plan said four synthetic posters. There are seven. dean, dinersclub and message-worker send the triple here via local structs, following dean's existing pattern rather than publishing botparty. message-worker was ALSO a second producer of chat-events: emitWhatsAppEcho published directly, bypassing hermes, with no envelope -- so every WhatsApp send would have minted a permanently NULL-account row. It now stamps from cmd.PlatformAccountID, and both exported publish methods funnel through one private publish() that inspects serialized bytes, so the guard holds for shapes nobody has written yet. Reports by default; refuses only under STRICT_EVENT_ENVELOPE, because dropping the echo stalls every WhatsApp survey. Missing user is now 400 rather than 500, so the /synthetic contract is coherent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
Four places where two conversations for one participant collided and a row silently vanished: - DedupStates keyed its map on UserID alone, so a batch holding state for one user on two accounts kept ONE. Now keyed on (UserID, Pageid). - responses and chat_log ON CONFLICT targets excluded the account, so the second account's row was treated as a duplicate of the first. Widened -- which required migrations 27/28, because those targets ARE the primary keys. - exodus bail targeting built its CTEs over responses aggregated across ALL accounts and joined them to account-scoped states rows on userid alone, so answers on number A could qualify a participant for a bail on number B. scribble now reads account_id/platform from the envelope for messages, with the historical extraction kept for the backfill so both read one rule -- TestBackfillSQLMatchesGo evaluates the actual .sql against the shared fixture. exodus conditions-based bails send COALESCE(s.platform, 'messenger') AS platform. The alias is load-bearing: executor.go reads row["platform"], and an unaliased COALESCE lands under "coalesce" and silently does nothing. The default is exact -- every states row on a whatsapp_business account carries a non-NULL platform, so all 1.07M NULLs are genuinely Messenger. Note exodus needs `go test -p 1`: query/ and db/ both DELETE FROM the same tables in setup and truncate each other's fixtures. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
This is the bug. State lived at `state:<userid>`, and on WhatsApp a user id is a phone number -- identical across every business number a participant messages. Two conversations shared one blob, last write won. Reproduced live 2026-08-16: an ad entry on one number wrote the key; a button press on another read it back, answered a field from the wrong survey, and the conversation went to ERROR permanently -- that tag is not in DEAN_ERROR_TAGS, so nothing retried, and every touch refreshed the 24h TTL. The integration suite now reproduces it in both directions and the mechanism is worse than documented: the two surveys STITCH, and one researcher's field ref ends up holding another's answer. The key is now the full triple, and the three md fallbacks are gone. The plan listed one; there were three. transition.js:27 was the worst -- that value builds getForm() AND the outbound command, so a bled state routed replies to another researcher's page. conversationFromRawEvent reports what the event carried rather than deciding: an account with no platform still scopes the replay, because get() takes no platform. The strict gate discarded an account the event gave us, and an unscoped replay reads the OLDEST 30k events -- so it does not merely interleave, it can silently truncate. Also here: - chatbase-postgres absorbed into lib/chatbase. One implementation, two in-repo consumers, and a publish step that made the integration suite unbuildable (npm ci died on ETARGET). The testrunner was pinned four versions behind production. - DEFER: a synthetic event whose replay comes back empty no longer blank- starts onto FALLBACK_FORM. It returns without newState, because publishing ANY state here would UPSERT over the conversation's real states row -- the row every recovery sweep selects on. Leaving states alone IS the retry. - The REFERRAL case had no START guard, so a bare get_started arriving after an ad referral pushed 305 onto a LIVE stack. 3,732 conversations since 2020. Guarded on forms.length and on whether the REF named a form, so explicit ?ref=form.305 referrals still work. - clear-state-cache.sh matches the new key shape via SCAN, never KEYS. - Dead code deleted: stateman, Responser, scratchbot, batch -- all called a three-arg machine.transition against a two-arg method. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
linksniffer and moviehouse are ours, but a researcher authored their URLs -- including the account id. 465 of 570 moviehouse fields hardcode one, 63 of those are junk, and on 2026-08-13 one routed a WhatsApp participant's video event to a phantom conversation on a Facebook page keyed by a phone number. It is still BLOCKED in production. The first attempt had replybot DECORATE those URLs behind a `tracked` flag, which forced it to know which hosts are ours and what each service calls its params. That allowlist rotted twice -- and both hosts it rotted on (gbvlinks.nandan.cloud, virtuallab-videos.netlify.com) turn out to be dead: one 404s, the other is claimed by no Ingress and serves the controller's fake certificate. A list of hostnames inside a message translator cannot track reality. That design is deleted, not deprecated. Instead: `link_tracking` and `moviehouse` are field types. The researcher writes `type: moviehouse` + `videoId:` and nothing else; replybot builds the whole URL from config plus the conversation. No allowlist -- it uses its own host. No per-service param names -- one canonical vlab_* set, which makes the id collision (participant vs Vimeo video) structurally impossible rather than narrowly avoided. No flag -- the type IS the opt-in. And no way to author a wrong account id, because the researcher never authors one. Hand-authored webviews are untouched: no matching, no decoration, byte- identical output. Migration is "change the type", not "add a flag". Missing config throws at translation with no tag, so it routes to STATE_ACTIONS -- a platform fault, and in DEAN_ERROR_TAGS, so stranded participants are retried once it is fixed. Startup refusal would crash-loop all 8 replicas over one field type. linksniffer also always redirects now: it previously swallowed the redirect when the event POST failed, so any hermes blip broke every tracked link. Also: dashboard-server dual-writes the messaging_accounts registry in one transaction, and rejects malformed messaging creates. A blanket rejection would have broken both live connect flows -- there is one create path, not the two the plan assumed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
It could not. One researcher, one page, one WhatsApp number -- and the facebot mock bucketed outbound messages by user id alone, so two conversations for one participant were literally indistinguishable to the assertion helpers. On Messenger the only account signal on the wire is the bearer token, and every account shared the token 'test'. Now: two researchers, four accounts, a conversation-keyed receiver, account- scoped DB readers, and the messages + chat-log scribble sinks the stack never ran -- without the messages sink the event log was always empty, so every replay assertion would have passed vacuously. 61 tests, including the 2026-08-16 reproduction in both directions and the message_pointer leak, which was untestable for a reason nobody had noticed: REPLYBOT_RESET_SHORTCODE is set in staging and production but never in kube-dev, so form.reset -- and the whole pointer half of the replay -- could not fire here at all. Harness defects found and fixed while proving the above: - consumeTopic read the OLDEST 500 records; the topic passed 556 mid-suite. Replaced with a bookmark, so it is immune to suite growth rather than sized for today. - KEEP_STACK never worked. Two bugs: mocha applies its timeout to hooks, and an awaited promise does not keep node alive (signal handles are unref'd). The documented debugging workflow tore down the containers it promised. - WhatsApp E2E is also mocha.parallel, which the README denied -- and is why two runs of identical code gave 50/11 and 49/13. B3-1's failure diagnostic now dumps every row with its pageid, so "written on the wrong account" and "the sink is behind" can never be reported as each other again. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
The plan document is preserved with its original reasoning intact and each wrong claim marked CORRECTED beside it, so nobody re-derives a conclusion the evidence already killed. It was wrong about: the migrations §7.2 needed, the premise of §7.4, the number of synthetic posters (four, then six, then seven), the number of md fallbacks (one, then three), whether hermes is the only producer, and the platform split in §1.3 -- which presented an inference as a measurement. The one general lesson, in the test plan's corrections register: A gate is only tested if something tests the thing that feeds it. Where a test constructs its input by hand rather than obtaining it from the real upstream, it proves the consumer handles that shape -- never that the shape occurs. That is how conversationFromRawEvent shipped with no coverage while a test handed the state store the exact object it could never produce, and how a second producer of chat-events existed unnoticed. Every problem in this effort had passing unit tests on both sides of an unwatched seam. New: event-envelope.md (the wire contract and its seven producers), messaging-accounts.md, and a rewritten questions.md -- the researcher-facing contract, which had never documented that rich field types are authored in the Typeform description box at all. Still open, listed in the plan's §8: the pageid -> account_id rename, the chat-log publisher deleted in a refactor on 2026-07-26 (exports have been silently truncating since), hermes verify-mode platform resolution, and the researcher migration to the new field types. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
responses.platform existed since migration 26 but nothing wrote to it. The platform was already in scope at actionsResponses (transition.js:83) and simply never reached responseVals -- three files, no schema change. An absent platform is SQL NULL, not the empty-string sentinel used for pageid. responses.platform is nullable and not part of the primary key, so it can say "unknown" honestly without grouping every unattributable row under one fake platform -- the same nullIfEmpty precedent messages uses. The column exists for the credentials-cascade guarantee: a deleted researcher would otherwise strip the platform binding from archival history, and the account id alone cannot recover it once the credential is gone. Until this change that guarantee did not hold. chat_log.platform has the same gap but its producer was deleted in a refactor on 2026-07-27; wiring that column goes with the restore. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
✅ Deploy Preview for vlab-research canceled.
|
✅ Deploy Preview for virtuallab-videos canceled.
|
The test-db target starts CockroachDB with docker run -d and immediately pipes migrations into it. CockroachDB needs a few seconds to initialize before it accepts connections, so the SQL command hits 'cannot dial server' and fails. Added a retry loop that waits up to 30s for the server to accept a connection before running the migrations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The conversation triple never needed this table. replybot reads (platform, account_id, user_id) off the event envelope that hermes stamps at ingest -- there is no lookup on the path this PR fixes. The registry was solving a different problem and solving it by inference. WHAT WAS WRONG WITH IT. `entityToPlatform` guessed a credential's platform from its entity, because the create endpoint was never told one. That is a one-time MIGRATION concern -- existing credentials predate the column -- promoted into permanent business logic, where it then had to justify itself: the "entity -> platform is not a function" argument, the Instagram case, and a documented reversal of the ratified (allocator, id) decision, all to defend a value we were making up. The user tells us the platform. They pick what they want to message on, then which accounts serve it. Platform is user input on a form, not a derivation, and until that form exists the table holds guesses that nothing reads (documentation/messaging-accounts.md said so outright: "Nothing reads messaging_accounts"). WHY THE TOKEN PATH DOESN'T NEED IT EITHER. credentials is keyed (entity, key) where key IS the account id, and messaging account ids are globally unique (verified in production: zero duplicate keys). So a bare account id resolves to exactly one credential without knowing the platform -- which is what tokenstore.go's fallback already does. If a page is connected, we can send on it; what platform an event belongs to is determined by the webhook it arrived on, not by user intent. WHERE THIS BREAKS, AND WHEN TO BRING IT BACK. Instagram. Its webhooks carry the Instagram account id in recipient.id, not the Page id, so the derived id matches no credentials.key and the lookup finds nothing -- the token exists (it is the Page's), we just cannot reach it from the id the event gave us. That is a mapping problem, instagram:<ig id> -> the Page credential, and it is exactly this table's row shape. So the registry is the Instagram fix, and only that. It returns when Instagram or the connect-accounts UI needs it, populated by what the user tells us. The full implementation is preserved on origin/archive/messaging-accounts-registry (migration 25, the dual-write transaction, the pure decision layer, 42 tests, the sql-exporter collectors and documentation/messaging-accounts.md). Removed: migration 25; credentials.core.js and its tests; credentials.test.js; createWithMessagingRegistry and the controller's transactional path; messaging_account_health collectors; documentation/messaging-accounts.md; the reversal note in documentation/platform-abstraction.md (the ratified (allocator, id) decision stands unreversed). Also drops the two dangling references to hermes "verify mode" in documentation/event-envelope.md -- an unbuilt feature that only existed to check events against the registry. Migration numbering skips 25. Migrations apply lexically and 26/27/28 have no dependency on it, so the gap is inert.
Three read-only investigations checked every disputed assertion against the
source before anything was edited. Two claims were false, and both had been
repeated widely enough to look like established fact.
FALSE CLAIM 1: "FALLBACK_FORM=305 is another researcher's survey."
formcentral/db.go:77-88 resolves a survey with
WHERE s.userid=(SELECT userid FROM credentials WHERE key=$1
AND entity IN ('facebook_page','whatsapp_business') LIMIT 1)
AND s.shortcode=$2
-- pageid to owner FIRST, then shortcode within that owner. `surveys` has no
uniqueness on shortcode alone; the scoping index is (shortcode, userid,
created DESC) (01-init.sql:53). Two researchers can each own a `305` and
formcentral cannot confuse them. This document's own §1.2 already proved it:
shortcode `hpvbl` sent to the 541 page 404s precisely because it belongs to
the owner of the 202 page.
So the misroute never crosses a researcher boundary. It is still severe --
the participant is pulled off their real survey onto a different survey in
the SAME account and their answers are misattributed, which still looks like
a completion rather than an error. Only the ownership claim was wrong, and
it borrowed its vocabulary from a genuinely cross-researcher bug: the
unscoped Redis key `state:<userid>`, which is what this branch fixes. Those
sentences were verified separately and left alone.
Corrected in 19 places across replybot/, documentation/ and planning/.
FALSE CLAIM 2: DEFER's justification named the wrong mechanism.
The comments said `_noop()` would clobber the conversation's real states row
and that bumping `updated` is how the harm lands. Verified against
dean/queries.go: Timeouts() and Payments() never reference `updated` at all
-- they key off calculated_timeout_date and state_json->>'waitStart'. Only
Respondings/Errored/Blocked/FollowUps read it, and bumping it delays or
extends eligibility rather than destroying anything.
The two DEFER sites also are not the same case:
* Site A (synthetic event at START). dean reads the account off the very
`states` row it re-fires for, dinersclub off the issued payment, so the
account is provably correct and a START replay can only be stale (Redis
TTL 24h vs DEAN_TIMEOUT_MAX_PAST=72h). Publishing it flips `current_state`
off WAIT_EXTERNAL_EVENT on the real row -- which is exactly what
Timeouts()/Payments() gate on. THAT destroys the retry. moviehouse and
linksniffer read a researcher-authored pageid off a webview URL, so a
wrong account hits a different (userid, pageid) key and cannot clobber
anything.
* Site B (form-less entry on a live conversation). A real webhook, always
account-correct, so `_noop()` would UPSERT byte-identical content.
DEFER here is hygiene, not safety, and now says so.
The comments were also far too long. The transition.js block went 44 lines
to 24; machine.js and transition.test.js were cut to match.
PLAN RESYNC (registry removal, 5c4cab3).
planning/conversation-identity.md 2337 -> 2189 lines: §5's registry section
collapsed to a pointer at origin/archive/messaging-accounts-registry, §7.6
rewritten, the status header corrected, and 7 dangling references to the
deleted documentation/messaging-accounts.md removed. §5.1's six-consumer
inventory is still true of live code and was preserved as background.
planning/conversation-identity-test-plan.md: §B11's ~90-line spec for a table
that no longer exists deleted and replaced with a pointer; §0.9 item (2)
preserved verbatim with a SUPERSEDING NOTE per this document's own convention
-- the reversal it recorded was itself reverted, so the ratified (allocator,
id) decision stands unreversed; the §0.8 migration recon dated and the current
migration set listed, including the inert gap at 25.
TWO ITEMS LEFT OPEN, deliberately not decided here. Both were justified BY the
registry reversal, which has itself been reverted, so the premise is gone:
whether the "same account id, two platforms -> two distinct keys" test is
load-bearing or defensive-only, and whether §7.3 must remain a hard
prerequisite for §7.1. Marked **OPEN -- needs decision** in place rather than
re-derived.
No executable code changed -- comments and prose only, verified by diffing
out comment lines. replybot 637 passing / 1 pending, dashboard-server 489
passing, both matching baseline.
Removes the DEFER action, both DEFER_* constants, both greppable log tags, the `apply` case and the recovery table. Both refusal sites now return _noop(). DEFER was two days old, never merged, never deployed, and narrowed a fix from 3de533a that is live on main. Two of its three justifications did not survive tracing: START is a reachable state of a live conversation (apply's RESET, 1,623 production rows), and dean does not re-fire after such a refusal -- dean/queries.go:196 gates on current_state = 'WAIT_EXTERNAL_EVENT', which a reset has already cleared. Behaviour is unchanged in what matters: a synthetic event at START still starts no form and sends no message. One deliberate difference: _noop() returns newState, so lib/index.js:100 publishes and caches the unchanged state rather than writing nothing. What this gives up: both log tags. Neither refusal rate is measurable from pod logs any more. That was an explicit decision -- the substitute is the `states` detector queries in documentation/referral-form-resolution.md. Also trims the _wrapPayment comment in the same file, part of the branch-wide comment sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
Comments only, no logic. Three places claimed that falling back to form 305 could put a participant in another researcher's survey. It cannot. Shortcodes are user-scoped, not global. formcentral/db.go:82 resolves a survey by `s.userid = (SELECT userid FROM credentials WHERE key = <pageid>)`, i.e. by the owner of the account the conversation is already on, so FALLBACK_FORM always names a survey inside that account. 52 survey versions carry shortcode '305' across 11 distinct accounts (prod, 2026-08-20). The real harm is misattribution WITHIN one account -- the participant lands on that account owner's fallback survey instead of the survey the ref named, and then counts as activity there. That is also why the sql-exporter CASE was wrong: it collapsed 11 accounts' real studies into one "fallback (no study)" label. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
417 lines of untested bash become a Go module with 24 tests (10 unit, 14
integration against a real CockroachDB). The extraction rule is unchanged and
still lives in one place: devops/sql/*-expr.sql, which scribble's
TestBackfillSQLMatchesGo also evaluates, so SQL and Go cannot drift.
The expressions still run SERVER-SIDE. `content` is 384 GiB and never crosses the
wire; this process issues UPDATEs and moves a cursor, nothing more.
Three real bugs fixed in the move:
- the cursor was interpolated into SQL text and quote-escaped with sed; it is
now parameterized, and a userid is not guaranteed to be numeric
- errors were detected with `grep -qi 'error'` over psql output; now pgx errors
- json_valid(content) was evaluated a third time in the predicate to learn
nothing the account expression's own first branch had not already decided
Adds --dry-run (counts only) and --rehearse (runs the real UPDATE in a
transaction, then rolls back).
Mutation-checked rather than merely green: dropping `AND account_id IS NULL`
fails 3 tests, and turning the batch bound `<=` into `<` fails 4, including rows
silently skipped.
Also drops two claims that deferred to a test suite which did not exist -- it
exists now -- and fixes a chatbase.js pointer to a "section 4" of migration 26
that has no sections.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
The 12 fixture vectors were a single #[test] with a hand-rolled loop: 12 vectors reported as 1 test, and the first failure aborted the other 11. Each is now its own named test, grouped by the rule it pins, plus `every_vector_is_covered`, which fails if a vector is added without a test. That guard was verified to actually fail by deleting a name. Rust was the odd one out here -- JS already does one it() per vector, and Go uses t.Run subtests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
One value, "false" -> "true", plus its comment. The file's own comment already said STAGING IS WHERE THIS GETS TURNED ON FIRST and it had never been done. Turned on deliberately for the account_id migration: an envelope omission that merely logs is one nobody chases, and the point of the migration window is to surface producer bugs immediately, while someone is watching. A stalled staging survey is the loud failure we want. PRODUCTION STAYS "false". There, refusing drops the WhatsApp send echo, which is the only thing advancing those conversations. Flip production only after CHAT_EVENTS_ENVELOPE_MISSING reads zero in staging for 24h, as its own diff. NOT APPLIED. This commit is half the change; it takes effect on helm upgrade gbv vlab -f devops/values/staging.yaml -n vstag kubectl rollout restart deployment/gbv-message-worker -n vstag Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
-3,728 lines. Removes the reverted messaging-account registry, the DEFER machinery, a 651-line "corrections to the other document" chain, the superseded moviehouse allowlist design, and every ~~RESOLVED~~ strikethrough section. THIS OVERRIDES THE DOCS' OWN STATED CONVENTION of preserving superseded reasoning verbatim. That was an explicit instruction, not an oversight: the accumulated corrections had grown longer than what they corrected, and two claims had hardened into apparent fact purely by being written down repeatedly. Adds a rollout runbook (§5), the four log signals to watch, the current feature gate values, per-migration rollback, and planning/messages-account-not-null-todo.md for the account_id NOT NULL work that is deliberately not in this branch. Production measurements now carry their date and their method, so the next reader can re-run them instead of trusting them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
MissingEnvelopeFields unmarshalled into map[string]json.RawMessage and looked up each field by name; it now unmarshals into a two-field struct. Same contract: still inspects the SERIALIZED BYTES rather than a typed command, so it stays a chokepoint for every shape this service publishes; still counts a field present only when it is a non-empty JSON string; still names both fields on an unparseable body. ONE BEHAVIOUR DELTA, deliberate but worth naming rather than burying in the comment sweep this was originally grouped with: encoding/json matches struct tags case-insensitively, so a body carrying "Account_ID" now satisfies the guard where the map lookup required an exact "account_id". No producer emits mixed case -- hermes, emitWhatsAppEcho and types.UniversalEvent are all lowercase -- so this loosens a guard nothing currently trips. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
Comments only, plus test names and assertion message strings. No logic anywhere
in this commit -- verified by filtering the diff to non-comment lines.
Removed:
- every planning-document section-number reference this branch added (~40).
Code should not depend on a doc's numbering to be read, and §7.4 had already
outlived one renumbering.
- all changelog prose -- "OBSERVED PRE-FIX FAILURE", "This used to read...",
and the account of what each comment previously said. That belongs in git.
- `[RED until §7.1]` in TEST NAMES, which told a reader that passing tests were
expected to fail.
- content that belongs in the planning docs, not next to the code.
Representative: 26-messages-account.sql 208 -> 62 comment lines, chatbase.js
108 -> 32.
One change that looks functional and is not: scribble/account.go was branch-new
and failed gofmt, because Go 1.19+ wanted to rewrite the '' sentinel in a doc
comment into a typographic quote -- which would have made the comment describe a
value that does not exist. Reworded to "the empty-string sentinel".
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
Struct-tag alignment only, no code. Commit 36808aa added SyntheticEvent.Event and a longer field to main.go without re-running gofmt, so `gofmt -l` reported two files that were clean on main. The five files still listed by `gofmt -l` across dean, scribble and message-worker were already unformatted on main and are deliberately left alone. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
§4 gains the one piece of genuinely new work this branch leaves behind: nothing runs the Go suites. scribble, dean, message-worker and now devops/backfill have never run in CI, which is not a regression but does undercut the argument for moving the backfill off bash. .github/workflows/replybot-test.yml already has the pattern, including `make -C ../devops test-db`. §9 said forty code comments still cite the old numbering. 3ec2e27 removed them; `grep -rn '§[0-9]'` over the source tree now returns nothing. The map stays for reading the branch's history, but its stated reason for existing was no longer true. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
Brings 29 commits of main onto the branch. Required before anything here can be applied: the branch's values files predated four version bumps and would have rolled staging and production BACKWARDS (replybot v0.0.219->218, dean v0.0.45->44-wa, dinersclub v0.0.46->45, message-worker v0.1.19->18). Both values files auto-merged correctly: main's version anchors, this branch's STRICT_EVENT_ENVELOPE=true in staging. Verified anchors are now identical to main's. Seven conflicts. The two that were not mechanical: replybot/lib/event-normalizer.js -- both sides rewrote WHATSAPP_ENTRY_REF and _refFromText for DIFFERENT real bugs. main widened the token alphabet to accept percent escapes (CTWA metadata values contain spaces); this branch made the `form` pair position-independent (a CTWA autofill reads `creative.3b.gender.men.form.hpvintrotriple`). Taking either alone silently reintroduces the other's misroute to FALLBACK_FORM, so the resolution is the union: main's alphabet inside this branch's leading-pairs/shortcode/trailing structure, plus main's disjoint `r.<base64url>` encoded anchor checked first. Verified against 11 cases drawn from both sides, including each side's regression case and the rejections both rely on. dinersclub -- main deleted TestDinersClubAuthError (AUTH_ERROR is now withheld as a precondition, replaced by TestAuthFailureIsWithheld in recovery_test.go); this branch had only re-plumbed it botparty -> Poster. Took main's deletion, then ported recovery_test.go onto the branch's Poster, which main's version could not compile against. Also fixed, not a conflict but a real interaction: main's new `ad_id projection` test seeds `responses` without a pageid, which migration 28 makes part of that table's primary key. Both inserts now supply one. Suites on the merged tree: replybot 698, dashboard-server 494, hermes 39/7/34, scribble, dean, dinersclub, message-worker, backfill 24, facebot tsc. All green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
`go build` drops its output at scribble/scribble, and 2940322 committed it. It is not on main or staging -- this branch is the only place it exists, and it blocked the merge into staging by trying to land a compiled artifact there. scribble/.gitignore was present but EMPTY, which is why nothing caught it. Same class of mistake as the devops/backfill binary that was caught during review, in the one directory nobody thought to check. The blob stays in this branch's history (2940322 is already pushed); dropping it from there means rewriting a branch with an open PR, which is a separate call. This stops it reaching staging or main as a live file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
Integration test B8-6 greps this document for the literal marker "NULL-ACCOUNT-ID TOLERANCE REMOVAL CONDITION" and fails when it is missing. The docs rewrite (697f09b) deleted the marker while the `OR account_id IS NULL` clause it documents is still live in chatbase.js -- exactly the "undocumented permanent-looking migration hack" the guard exists to prevent. Caught by testcontainers-integration on the PR (60 passing, 1 failing), not locally: that suite needs testcontainers and takes 12 minutes, and it was not in the handoff's verification list. Everything I ran locally was green. Restored with the removal query, the consequence (drop the clause, delete B8-5a and B8-6, tighten B8-5b), a pointer to the guard itself, and the caveat that a plain NULL count never reaches zero because ~3,000 synthetic rows carry no account at all. Swept the repo for other tests that assert on documentation content: B8-6 is the only one. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YMuYmd7FTBNoaaiVnNEhh2
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
1 participant
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.
Replybot keyed conversation state by user id alone. On WhatsApp a user id is a phone number, identical across every business number a participant messages, so two conversations shared one state blob — last write wins, conversations die permanently, and one researcher's participant data lands in another's account scope.
A conversation is (platform, account_id, user_id). This PR makes every layer carry the triple.
What changed
Test results
Integration (testcontainers): 61/0. replybot: 637/0. message-worker: 84/0. exodus: 105/0. scribble: 113/0. hermes: 68/0. dashboard-server: 531/0.
Migrations (files only, not applied)
25 (registry) and 26 (messages columns) are additive. 27 (chat_log PK) is low-risk — table dormant since 2026-07-27. 28 (responses PK + 1.8M-row backfill) needs review — 39 GiB, the only hot table. Sequencing: 27 then backfill then 28 then scribble deploy. Runbook in each migration header.
Not in this PR
pageid rename (cosmetic, own PR). chat_log producer restore. SYNTHETIC_REQUIRE_CONVERSATION gate (stays off). Researcher migration to new field types. Backlog: VIR-28 through VIR-31.
Generated with Claude Code