Reviewer dimension: architecture · correctness · robustness · security/privacy · test quality
Code reviewed: origin/staging @ 554c6a8d (snapshot in ../src/ is byte-identical to staging for all 41 touched files; verified with cmp), commits 783116cc → e81dbc03 by Sabeena.
Live evidence used: Axiom digital-coach-logs (service digital-coach-staging, 13–27 Sep), read-only analyst queries on the shared Supabase DB, anonymous + presigned HEAD probes of the R2 objects, and a jest run in a throwaway worktree (since removed).
Bead: bd-ak1go
The shape is sound. It is a flag-gated, coach-marked (Track A only) state machine split into small, single-purpose services, with deterministic stimulus rendering and careful "never hijack" claim guards. The flag-off path is truly inert. But the build does not work end to end on staging today, and it is not safe to promote.
anon/authenticated hold SELECT/INSERT/DELETE. Every sibling assessment table has RLS on.Tests are plentiful (132 pass). They cannot catch any of the above: WhatsApp and Supabase are mocked to always succeed, image URLs are checked only for shape, and the voice routing test greps source text.
What recordSubtestAttempt does today when a listening-comprehension voice note has been captured
handleEgraEgmaIctTestVoiceNote always sets whatsappMediaId and recordedAt, even when the R2 upload failed, and sets url when the upload succeeded. It stores these as state.pendingVoiceNote (shared/handlers/egra-egma-ict-test.handler.js:786-791).recordQaAnswer passes audioUrl, audioWhatsappMediaId and audioRecordedAt (:446-448).results.service.js:57-59 spreads each of those into the insert payload whenever it is non-null. So a listening row that has a voice note always carries at least audio_whatsapp_media_id and audio_recorded_at.audio_* columns exist in the live table. Postgres rejects the statement: an EXPLAIN INSERT … (audio_whatsapp_media_id) run under the read-only role returned ERROR: column "audio_whatsapp_media_id" of relation "egra_egma_ict_test_attempts" does not exist. The whole row is refused, not just the audio columns.results.service.js:64-69 logs logToFile(..., 'error') plus a persist_failed event containing only error.message and subtestKey. It does not log the scores, the item verdicts or the sessionId. The module doc's claim that it "logs the full result" (results.service.js:15) is false.handler.js:434-451), moves on to the next subtest, and later sends "✅ Assessment complete" with the scores from Redis. Then it clears the session (handler.js:338-348). The coach is never told.What is lost per full administration: both listening-comprehension rows, listening_comprehension_urd (subtest 1) and listening_comprehension_eng (subtest 6). Each loses the coach's 6 item verdicts, the correct and incorrect counts, the audio link and the media id. Once the session is cleared these exist nowhere: not in the DB and not in the logs.
The audio file itself is orphaned in R2 at egra-egma-ict-test/voicenotes/<session_id>/<subtest>.ogg. It can be found only through the separate voice_note_captured event, which does carry sessionId.
The other 12 subtests are written correctly. Their payloads carry no audio_* keys, which is exactly what the conditional spread was designed for. The design protects every row except the one the pilot exists to collect.
TESTING.md is wrong about this. It says the pilot "is fully inert" until both the Flow is registered and the migration is applied (TESTING.md:239-242). The code gates only on EGRA_EGMA_LISTENING_FLOW_ID (handler.js:311). It never checks the migration.
Why only one row exists (reconstructed from Axiom):
A row is written only when a subtest completes. No session-start row exists.
| Session start (UTC) | What happened | Row? |
|---|---|---|
| 17 Sep 08:08, 09:58 | /egraegmatest not handled by EGRA (gate failed) |
no |
| 18 Sep 04:13 | started, no class ever picked | no |
| 18 Sep 06:28 | roster pick → listening_comprehension_urd on the old plain-text path (the Flow commit 5fd2da1e landed 06:51) → subtest_recorded at 06:30:35 → next grid never scored |
1 row (3/3/0, student_id NULL) |
| 18 Sep 06:37–10:47 (7×) | /egra from a NULL-region tester → general chat (the bd-eg7st loop) |
no |
| 18 Sep 12:31, 12:32, 12:36 | Flow sent; submission logged ⚠️ Received unknown/unrecognized flow submission (12:32:06, bd-2711, before the fix deployed) → tester typed /cancel |
no |
| 18 Sep 13:55, 19 Sep 04:48, 21 Sep 18:56, 24 Sep 07:06 | Flow completed → sendImageFromUrl failed (passage image) → voice-note prompt sent → no voice note ever sent, session abandoned |
no |
There are zero voice_note_captured and zero persist_failed events in the window. So the audio-insert failure has not happened yet. It is guaranteed on the first session that gets past the voice note. The one row is not the result of loss. It exists because only one session ever completed a subtest.
Consequence. For letter ID (eng/urd), familiar words (urd) and ORF (eng/urd), the coach receives only "Ready? ▶️ Start". There is no letter grid, no passage, and no instructions, because the caption rides on the failed image. The coach has nothing to show the child. In the voice-note step, the passage that bd-2712 was meant to keep on screen never appears.
Evidence.
- All 7 URLs use the S3-API host: shared/config/egra-egma-grid-images.json:3-7 and egra-egma-passage-images.json:3-4, for example "https://58a6fb2a86d61397895d6b97b73a3ebe.r2.cloudflarestorage.com/digital-coach-audio/egra-egma-ict-test/stimulus-images/letter_id_eng.png".
- sendImageFromUrl does an unauthenticated axios.get(imageUrl, …) (shared/services/whatsapp.service.js:1122). An anonymous GET of those URLs returns HTTP 400 <Error><Code>InvalidArgument</Code><Message>Authorization</Message>, which I reproduced for both a grid and a passage URL.
- Live staging: ❌ Error in sendImageFromUrl fired on 19, 21 and 24 Sep, 3 of 3 attempts. The 24 Sep row shows remoteHost: 58a6…r2.cloudflarestorage.com, statusCode: 400, and the body decodes to the same R2 InvalidArgument/Authorization XML.
- The return value is dropped, so the "never a hard-stop" text fallback (which only triggers when the URL is null) cannot rescue it:
- handler.js:163 await WhatsAppService.sendImageFromUrl(from, imageUrl, caption);
- handler.js:276 (passage), same pattern.
- A presigned HEAD shows all 7 objects exist (119–220 KB PNGs). The fix is to presign, or better to upload once and cache WhatsApp media IDs, exactly as PROJ-42 already does: proj42-session.service.js:370 getPresignedUrl(buildR2PublicUrl(q.image_r2_key), 3600). Also branch on false so the plain-text fallback actually runs.
- No grid image send has happened on staging yet (no session reached a grid after b1a2513e). The grid failure is proven by reproducing the same code path against the same URL shape.
Consequence. Covered in full above: both listening rows are lost per session, the scores are unrecoverable, and the coach is told the assessment is complete.
Evidence.
- results.service.js:58 ...(row.audioWhatsappMediaId != null ? { audio_whatsapp_media_id: row.audioWhatsappMediaId } : {}),
- handler.js:447 audioWhatsappMediaId: pendingVoiceNote ? pendingVoiceNote.whatsappMediaId : undefined,
- Live DB (read-only): the table has 22 columns and no audio_*.
- Fix: apply pending_egra_egma_ict_test_voicenote.sql, or gate the pilot on the columns existing. Separately, see S11 for surfacing any failed persist.
Consequence. Letter-ID fluency is under-reported by twice the error count. A child who read 40 letters with 6 wrong in 60 s gets 28 instead of the standard 34 clpm. A child who is discontinued (0/10) is recorded and shown a negative rate: the test enshrines −27.3/min. This contradicts the team's own research spec, research/01_EGRA_EGMA_structure.md §1.3: Subtask_per_minute = Subtask_score / (Time given − time_remain) × 60, where the score is items correct, and "the child scores zero" on auto-stop. correct is already attempted − incorrect.
Evidence.
- shared/services/egra-egma-ict-test/rate-scorer.js:54 const correct = attempted - incorrect;
- :56-57 ? (correct - incorrect) / elapsedMinutes // RTI CLPM formula
- __tests__/rate-scorer.test.js:46 'handles a 0/10 first-row discontinue without crashing, producing a valid (possibly negative) rate'
- :19 expect(result.ratePerMinute).toBeCloseTo(28.0, 5);
- The cited source egra_reading_flow_redesign_plan.md §8/§10 (rate-scorer.js:8) does not exist anywhere in the project tree.
anon/authenticated can read, insert and deleteConsequence. The first migration has been applied to the shared prod/staging DB. The table stores children's names (student_identifier) and scores. Its only protection is the secrecy of the Supabase anon key. Every sibling table enforces RLS. This is the class of issue Supabase's security advisor flags as critical.
Evidence.
- Read-only query: pg_class.relrowsecurity = f for egra_egma_ict_test_attempts.
- has_table_privilege('anon', …, 'SELECT'|'INSERT'|'DELETE') returns t|t|t, and the same for authenticated.
- relrowsecurity = t for numeracy_assessments, reading_assessments, students, student_lists and exam_check_sessions.
- The migration has no ENABLE ROW LEVEL SECURITY anywhere (database/migrations/pending_egra_egma_ict_test_attempts.sql:26-95).
- The live row count is 1, and that row contains a child's name.
Consequence. The gate is only "flag on AND region ∈ {ict, islamabad, federal}". With the flag on, a teacher typing /egra sees a list of other teachers' names, schools and classes. Tapping one returns a named child from that teacher's roster. This is a cross-teacher disclosure of child PII, and nothing restricts it to coaches.
Evidence.
- access-gate.js:45-47 return isFlagEnabled() && isIctRegionUser(user); has no users.role check. Other features do check role, for example NEO on role='aeo'.
- roster.service.js:53-62 selects id, name, school_name, phone_number, region, student_lists (…, students ( id, student_name, roll_number, is_active )) for .in('region', Array.from(ICT_REGION_TAGS)), with no user_id scoping.
- Live DB: the ICT-region roles are teacher 53, coach 1, school_leader 1, cluster_coordinator 1. Seven classes and 41 active children are reachable today.
/cancel and every other command is swallowedConsequence. Three phases ignore /cancel:
- In awaiting_student_name, /cancel (or /egra, /menu) becomes the child's name and the assessment starts under that name.
- In awaiting_wrong_items and awaiting_stopping_point, /cancel gets "'/cancel' is not a number".
In every other phase, all slash commands except /cancel|/stop|/quit get "Please use the buttons above". So /menu, /status, /reading test and /egra are all dead for the life of the session, which is a 1 h sliding TTL. The hook runs before /menu (text-message.handler.js:2143). This breaks the repo's own convention: the numeracy and Support-Assistant guards deliberately let slash commands fall through (text-message.handler.js:370-391, "slash commands fall through so the user can always escape").
Evidence.
- handler.js:506-507 case 'awaiting_student_name': { state.studentIdentifier = text.trim() || 'Unnamed student';
- :513-517: the parse failure only re-prompts.
- :539-546: the /cancel regex lives only in default.
- text-message.handler.js:827: the EGRA hook sits ahead of /menu, /status and /reading test.
Consequence. Once the Flow is submitted, the only ways forward are to send some voice note or /cancel the whole assessment. That is contrary to "a failed voice-note capture must never block the actual assessment" (handler.js:752-754). Typed text in awaiting_voice_note or awaiting_listening_flow gets "Please use the buttons above", but no buttons exist in those phases. In all 4 real Flow-path sessions, the tester stopped exactly here.
Evidence.
- handler.js:545 'Please use the buttons above to continue the assessment, or type /cancel to stop.'
- No skip path exists for awaiting_voice_note anywhere in handler.js.
Consequence. The voice guard claims audio only in awaiting_voice_note. Any other child recording falls through to the normal voice route, where it is uploaded under audio/<phone>/…, transcribed by Soniox, stored in conversations, and answered by GPT. That covers:
- a recording sent before tapping "Ready to record" (awaiting_listening_flow),
- a retake after the first was captured (item_qa_in_progress),
- anything after TTL expiry or a Redis blip (S12).
This contradicts TESTING.md's "No SpeechAce, no Soniox … nothing". Typed names follow the same path. The bd-eg7st commit documents a child's name ("Sameer") going to GPT-5.4-mini when the gate failed. Every typed manual-path name is also written to Axiom.
Evidence.
- handler.js:766 if (!state || state.phase !== 'awaiting_voice_note') return null;
- voice-message.handler.js:884 "Step 1: Download audio", :955 "Upload audio to R2", :977 "Transcribe using language-aware ASR routing", :1133 voiceInboundConv = await storeConversation(
- text-message.handler.js:260 logToFile(`Processing TEXT message: ${messageBody}`);
- tests/bd-eg7st-…test.js:163 replays 'Sameer' into getResponseWithFormatChunked.
- No consent or retention step exists for recording children anywhere in the flow.
Consequence. All three marking buttons have fixed ids. A coach who scrolls up and taps ✅ on question 2's message while on question 4 records a verdict for question 4. Webhooks are deduped per message id only, with no per-user serialization, so two quick taps race on the read-modify-write session blob. One verdict is lost, or a duplicate prompt is sent.
Evidence.
- handler.js:207-209 Correct: { id: \${BUTTON_PREFIX}qa_correct` …(same id for every item)
-:404const item = qa.items[state.currentItemIndex];-whatsapp-bot.jswebhook:SessionService.isProcessed(message.id)is the only guard (whatsapp-bot.js:438`). No per-user lock exists in the file.
Consequence. The English passage has 60 words but 68 "items". The 8 full stops and commas are numbered cells that the child "reads correctly", which inflates cwpm and attempted counts by up to 13%. The Urdu passage is unaffected: its punctuation is attached to the words.
Evidence.
- item-bank.service.js:50 items: ITEMS.egra.oral_reading_fluency.eng.tokens, item_count: ITEMS.egra.oral_reading_fluency.eng.token_count
- Item bank: token_count 68, word_count_excl_punctuation 60, and the tokens include '.' ×7 and ',' ×1 (for example ['Imran','woke','up','early','for','school','.', …]).
Consequence. There is no 60 s cap and no "time remaining". The rate uses server Start→Stop time, so a late Stop tap (75 s) deflates the score by 20%. At the same time the caption tells the coach to "please self-time this".
The Discontinue button always records items 1–10 as wrong, for every grid. The rules it should follow are: - ORF: the item bank has no discontinue rule for ORF at all. - Familiar words: the instrument rule is "0-1 correct on the first row", so a child who got 1 right is recorded as 0. - Research spec: the first 5 items for word grids.
Evidence.
- handler.js:582-583 state.pendingElapsedSeconds = Math.max(1, Math.round(elapsedMs / 1000)); (no upper bound)
- :354 const elapsedSeconds = state.pendingElapsedSeconds || 1;
- :596 wrongItemNumbers: Array.from({ length: firstRow }, (_, i) => i + 1)
- :149 please self-time this
- Item bank: familiar_word_reading.urd.discontinue_rule = "If the child scores 0-1 correct on the first row (items 1-10)…"
Consequence. After a discontinued or partial ORF, every comprehension question still has to be marked, and the only choices are Correct and Incorrect. Unasked questions become Incorrect. The research spec requires "Incorrect vs no-response must be distinguished at item level".
Evidence.
- item-bank.service.js:65,69 set gatedOnGrid: 'orf_eng'|'orf_urd', but gatedOnGrid is never read anywhere in handler.js (grep: zero hits outside item-bank.service.js).
- Item bank reading_comprehension.*.response_options = ['Correct', 'Incorrect'].
Consequence. The chosen student's id lives only in Redis ("kept for potential future use"). The DB gets the name only, and names are not unique. The live row from a roster-picked session has student_id = NULL, although the column exists with an FK.
Random pick with replacement means the same child can be drawn repeatedly, and absent children can be drawn. The repo already has shared/services/reading/student-selection.service.js (bd-2203/2206): a coverage "staleness ladder", a present-today filter, an injectable RNG, and the reading_assessments.student_id FK pattern. The migration comment still says "This build does not integrate with the students/student_lists roster at all".
Evidence.
- handler.js:669 state.studentId = student.id; // additive — kept for potential future use
- recordSubtestAttempt payloads (:375-392, :434-449) have no studentId.
- roster.service.js:123 const idx = Math.floor(Math.random() * data.length);
- pending_egra_egma_ict_test_attempts.sql:40-44
Consequence. The new rule sits in _getBoundarySection, which is concatenated into every general-chat prompt (17 call sites: en, ur, ar, es, sw, bal/sd/ps/pa-PK, ta-LK). So every PK/TZ/YE/KE/PS general-chat reply changes. The redirect is wrong in places:
- Palestine: it sends teachers to /reading test, which is hard-disabled there ("Reading assessments aren't available here").
- Tanzania: it sends Kiswahili teachers to the English-clamped test instead of /reading test kiswahili.
- ICT EGRA users: it sends them to Rumi's auto-scored reading test, the thing the EGRA build exists to avoid.
"If a teacher mentions a student's name … do NOT start collecting details" is broad enough to redirect ordinary "how do I help Ali read?" questions. The actual trigger, /egra from an ineligible user reaching the LLM, is left in place.
The regression test only asserts that the string is present. The "end-to-end" test mocks OpenAI to return the desired answer, so no model behaviour was evaluated.
Evidence.
- openai.service.js:563 ## READING ASSESSMENT — NEVER RUN IT IN CHAT (CRITICAL — applies to ALL languages):
- Call sites :621, 649, 670, 689, 707, 725, 746, 762, 779, 796, 820, 844, 867, 891, 910, 931, 949
- text-message.handler.js:1038 PS gate "Reading assessments aren't available here."
- :1063 clampLanguage(…, ['en', 'ur', 'ar'])
- tests/bd-eg7st-…test.js:160-166 createMock.mockResolvedValueOnce(mockCompletion("… just type /reading test."))
Consequence. The problem goes beyond B2. The grid path tells the coach "Recorded: 18/20 correct…" whether or not the insert succeeded, and the summary always says "✅ Assessment complete". The failure log omits the scores and the sessionId. Nobody, whether coach, operator or data team, can reconstruct a dropped row.
Evidence.
- handler.js:375-397: await ResultsService.recordSubtestAttempt({...}); (result unused) then `Recorded: ${scored.correct}/${scored.attempted} correct`
- results.service.js:65-68 logs { error: error.message, subtestKey: row.subtestKey } only.
Consequence. The Redis client uses enableOfflineQueue: false, so during a blip every command fails at once. getState then falls back to a per-process Map that is empty for this coach. Buttons reply "No active assessment session", and typed answers such as "3,7" drop through to general chat. Writes during the blip go to memory only. When Redis recovers, the old state reappears.
With more than one web replica, the memory fallback is per replica. I could not see the replica count from the repo (see Unverified).
Evidence.
- session-state.service.js:28-36 falls back to memoryStore.get(phone) || null on any Redis error.
- railway-redis.service.js:63 enableOfflineQueue: false
flow-response.handler.js is dead code, and its dedicated test only tests that dead code. handleFlowResponse has no production caller (repo-wide grep finds only its definition and export). The real routing is whatsapp-bot.js → detectFlowType. That is how 5fd2da1e shipped "fully built and tested" but unreachable (bd-2711, per its own commit).flow-response.handler.js:204 if (flowId === EGRA_EGMA_LISTENING_FLOW_ID && EGRA_EGMA_LISTENING_FLOW_ID) {shared/handlers/__tests__/egra-egma-listening-flow-response.test.js mocks the EGRA handler plus 12 first-party modules.whatsapp-bot.js:2081 if (!ok) logToFile('⚠️ EGRA/EGMA Listening Comprehension flow completion not handled', { from });sendImageFromUrl is mocked to true (handler.test.js:15), and URLs are only shape-checked (:437 toMatch(/^https:\/\//), grid-image-urls.test.js:14). B1 is invisible.{ error: null } (handler.test.js:44). B2 is invisible.tests/handlers/voice-message-egra-egma-claim-guard.test.js:33 src = fs.readFileSync(HANDLER_PATH, 'utf8')). Root CLAUDE.md Rule 6 names exactly this anti-pattern.session-state.service and never runs the whatsapp-bot.js branch, which is where the bug was.tests/bd-eg7st-… and tests/egra-egma-listening-flow-completion-routing.test.js fail at import (Missing Supabase environment variables, because openai.service → bot-helpers → config/supabase throws at load). With dummy values, 16/16 suites and 132/132 tests pass.tests/bd-eg7st-…test.js:59 hardcodes a real tester's Supabase user id together with phone [number redacted] (:71).tests/egra-egma-listening-flow-completion-routing.test.js:7 includes the phone plus a name/class.TESTING.md:8 contains a personal absolute path /Users/sabeenaabbasi/….…attempts.sql:1), but it is applied.TESTING.md:4 says "Nothing in this branch has been merged, pushed, or deployed", but it is on staging.…voicenote.sql:38), but the bucket is private. An anonymous GET returns 400, which is good for privacy, but the comment is wrong./egra loads every ICT teacher's full student list. It also keeps each teacher's phone_number, which is unused, in Redis pendingClasses for up to 1 h (roster.service.js:56,81).handler.js:37,121). Today there are 7; the 10th class onward will silently be unreachable.egra-egma-ict-test/, the table egra_egma_ict_test_attempts, the Redis key and the egraict_ prefix. Hardcoded R2 account-host URLs sit in committed JSON config. EGRA_EGMA_LISTENING_FLOW_ID was added to the inventory test but not to the boot validator's EXPECTED_FLOW_ID_ENV_VARS (shared/services/flow-id-validator.service.js:23-50), so a wrong or unpublished id is not caught at boot.TESTING.md:216; Rule 20)..ogg with audio/ogg, whatever the incoming mime type (handler.js:778-779).coach_user_id has no ON DELETE rule; numeracy_assessments uses CASCADE.CHECK (rumi_auto_score_status = 'not_attempted_this_build') means a future Track B does need a migration, despite the comment at :74-75./egra handling has gaps./egra never restarts; it hits the phase handler./egra still goes to the LLM (the bd-eg7st trigger). A deterministic "not available for your account" reply would close that at the source.handleEgraEgmaListeningFlowComplete has no access-gate check. If the flag is turned off mid-session, the Flow completion still advances the session to awaiting_voice_note, but the (gated) voice handler no longer claims the recording, so it goes to the general pipeline (S4).handler.js:713-735.handler.js:459, 559, 629, 762). The flag is read fresh per call (access-gate.js:25-27), and tests cover text, button, list and voice with the flag off.rumi_auto_score_status is pinned by a CHECK constraint, verified live.buildGridHtml ↔ extractItemsFromHtml vs the item bank) proves the pixels come from the right text. All 7 objects exist in R2. Only the delivery is broken.egra-egma-ict-test/voicenotes/<session_id>/<subtest>.ogg contains no phone number or name.session_id, so a stale card cannot drive the session (handler.js:715-726).detectFlowType rule is narrow, sits before the greedy session_id fallbacks, and has false-positive tests (flow-type-detector.js:112).null unless the session is in the exact phase, so numeracy and coaching routing are unaffected otherwise.uploadBuffer, htmlToImage, sendFlow navigateData). The unreliable subtests (nonword, English familiar word) are excluded, with reasons recorded in the item bank.03_Rumi Portal, 04_Observability Portal) references it. This decides whether B4 is exploitable today or latent until the key leaks.railway.json. This decides whether the in-memory fallback (S12) is also a cross-replica split-brain.EXPLAIN under the read-only role. No write was attempted against the shared prod DB.