fix(remark): map indicator_ordinal → ordinal — no principal feedback has ever reached a teacher (prod cherry-pick) - #343
Open
hatafatif wants to merge 2 commits into
Open
fix(remark): map indicator_ordinal → ordinal — no principal feedback has ever reached a teacher (prod cherry-pick)#343hatafatif wants to merge 2 commits into
hatafatif wants to merge 2 commits into
Conversation
…ches a teacher loadScores selected `indicator_ordinal` and returned the rows untouched. computeS requires `ordinal` and throws on anything else, and that call sits ABOVE both of submitRemark's try/catch blocks — so the throw escaped the whole function and took the narrative, the teacher's coaching note AND the principal's confirmation with it. Scores still persisted (written before the failure) and the principal still saw the SUCCESS screen (returned before the async work), so the flow looked healthy while no teacher had ever received a note. remark-narrative-retry.worker.js has always done this mapping. The same query, mapped in one file and not the other, is the whole bug — and the worker is scheduled nowhere, so nothing drained the backlog either. Verified end to end by driving the real submitRemark: with the raw row shape it throws with teacher/principal/generated/marked all at 0; with the mapped shape both sends fire and deliveryPending is false. Tests: new remark-delivery-deps.test.js pins the boundary the two halves disagreed about — nothing exercised makeDeliveryDeps before, which is why 177 tests passed while production sent nothing. Full remark suite 182/182. Reported by Hasnat, 2026-08-17. Closes: bd-43488
`main` is failing its own logger-level ratchet — 390 live ❌-without-level callsites against a 389-entry allowlist — so every PR into main is blocked. The drift came in with 180370e, which added this callsite without re-baselining. Fixed by adding the severity argument rather than grandfathering it, which is what gen-logger-allowlist.js asks for: an unbound capture is a real failure and belongs at error, not info, where it maps to console.log and never surfaces as an error in Axiom. The allowlist is left untouched and now reports in sync, 0 new / 0 stale. Ratchet 3/3. No new observe failures against origin/main (8 pre-existing suites fail identically before and after).
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.
Production cherry-pick of
a118b57(fromdevelop, PR #342). Cherry-picked rather than merged becausedevelopis 301 commits ahead ofmainwith unrelated work.Reported by Hasnat, 2026-08-17:
Confirmed on production, and worse than reported — the principal's confirmation is lost too.
Root cause
remark-delivery.deps.jsselectedindicator_ordinaland returned rows untouched.remark-rubric.js:243requiresordinaland throws on anything else. ThatcomputeScall sits atremark-delivery.service.js:100— above both try/catch blocks — so the throw escapedsubmitRemark, taking the narrative, the teacher's note andsendToPrincipalwith it.It looked healthy because scores persist before the failure and the principal is shown
SUCCESSbefore the async delivery runs.remark-narrative-retry.worker.js:61has always done this mapping. The same query, mapped in one file and not the other, is the whole bug — and that worker is scheduled nowhere, so nothing drained the backlog either.Verified against
main's codeDriving the real
submitRemarkon this branch:indicator_ordinal)malformed score row (missing ordinal)—{teacher:0, principal:0, generated:0}ordinal){teacher:1, principal:1, generated:1},deliveryPending: falseRemark suite on this branch: 182/182. Cherry-pick applied with no conflicts — the remark files are byte-identical between
mainanddevelop.Blast radius
One line in
loadScores, plus a new test file. No schema change, no Flow asset change, no shared-service edit.WhatsAppService.sendMessageis untouched.Known follow-ups (NOT in this PR)
type: 'text'with no template fallback, so a teacher who has not messaged the bot in 24h will hit Meta131047. A teacher being evaluated is exactly the person unlikely to have messaged recently — this fix may relocate the failure rather than remove it. Check AxiomresponseData.error.codeon the first real submissions after deploy.Refs: bd-43488