Real-Telegram test suite green (11/11) + manual /add barcode OCR

.github/workflows/coupon-real-test.yml

@@ -58,6 +58,96 @@ jobs:
     - name: Checkout
       uses: actions/checkout@v4
 
+    - name: Validate COUPON_CI_BOT_TOKEN shape and identity
+      # Runs BEFORE the VPN/kubeconfig/image-build/deploy steps below so a
+      # bad token fails in seconds, not after a ~5-minute deploy. A 404/ok=false
+      # from getWebhookInfo or getMe means Telegram rejected the TOKEN — an
+      # unregistered webhook on a *valid* token returns http=200, ok=true,
+      # url="". This step never prints the token value, only its shape
+      # (length/whitespace) and the getMe result (username is not secret).
+      env:
+        BOT_TOKEN: ${{ secrets.COUPON_CI_BOT_TOKEN }}
+      run: |
+        LEN=${#BOT_TOKEN}
+        TRIMMED="$(printf '%s' "$BOT_TOKEN" | sed -e 's/^[[:space:]]*//' -e 's/[[:space:]]*$//')"
+        TRIMMED_LEN=${#TRIMMED}
+        HAS_LEADING_TRAILING_WS="no"
+        if [ "$LEN" != "$TRIMMED_LEN" ]; then HAS_LEADING_TRAILING_WS="yes"; fi
+
+        # Embedded newline/CR check — a raw value with internal line breaks
+        # reports a non-zero line count here.
+        LINE_COUNT=$(printf '%s' "$BOT_TOKEN" | wc -l)
+        HAS_EMBEDDED_NEWLINE="no"
+        if [ "$LINE_COUNT" != "0" ]; then HAS_EMBEDDED_NEWLINE="yes"; fi
+
+        SHAPE_MATCH="no"
+        if printf '%s' "$BOT_TOKEN" | grep -Eq '^[0-9]+:[A-Za-z0-9_-]+$'; then
+          SHAPE_MATCH="yes"
+        fi
+
+        echo "COUPON_CI_BOT_TOKEN shape: len=$LEN trimmed_len=$TRIMMED_LEN leading_trailing_whitespace=$HAS_LEADING_TRAILING_WS embedded_newline_or_cr=$HAS_EMBEDDED_NEWLINE matches_pattern(^[0-9]+:[A-Za-z0-9_-]+\$)=$SHAPE_MATCH"
+
+        HTTP_CODE=$(curl -s -o "$RUNNER_TEMP/getme.json" -w "%{http_code}" "https://api.telegram.org/bot${BOT_TOKEN}/getMe")
+        OK=$(jq -r '.ok // "null"' "$RUNNER_TEMP/getme.json" 2>/dev/null || echo "null")
+        USERNAME=$(jq -r '.result.username // ""' "$RUNNER_TEMP/getme.json" 2>/dev/null || echo "")
+        echo "getMe: http_code=$HTTP_CODE ok=$OK username=$USERNAME"
+
+        if [ "$HTTP_CODE" != "200" ] || [ "$OK" != "true" ]; then
+          echo "::error::COUPON_CI_BOT_TOKEN was rejected by Telegram (getMe http_code=$HTTP_CODE ok=$OK). Ranked hypotheses: (1) trailing newline/whitespace in the secret — the classic 'echo x | gh secret set' trap; (2) the secret is empty/unset; (3) the REVOKED token was stored instead of its replacement. Remediation: have the repo owner re-set the secret with: printf '%s' '<token>' | gh secret set COUPON_CI_BOT_TOKEN --repo Szer/bots (printf, not echo — echo appends the newline that causes this)."
+          exit 1
+        fi
+        if [ "$USERNAME" != "coupon_test_bot" ]; then
+          echo "::warning::getMe succeeded but result.username='$USERNAME', expected 'coupon_test_bot' — token may belong to the wrong bot."
+        fi
+
+    - name: Validate CI chat and bot membership
+      # Runs immediately after the token-shape/identity check above, still before the
+      # image build and AKS deploy, so a stale/wrong COUPON_CI_CHAT_ID also fails in
+      # seconds instead of surfacing 90+ minutes later as every real test's
+      # ensureCommunityMember gate silently rejecting the test account (see
+      # MembershipService.fs's IsMember — a getChatMember error and a genuine
+      # non-member verdict both collapse to `false` there, so this workflow-level
+      # check is the only place that can tell them apart without a src/ change).
+      # Telegram auto-upgrades a basic group to a supergroup (member-limit growth, a
+      # public link, or enabling "new members see history") and the chat id CHANGES
+      # SHAPE when that happens (bare negative id -> -100<id>) — dev-bot-settings.sql's
+      # COMMUNITY_CHAT_ID ('-5252116059') is a live example of this exact drift having
+      # already happened once for prod. This step never prints the token.
+      env:
+        BOT_TOKEN: ${{ secrets.COUPON_CI_BOT_TOKEN }}
+        CHAT_ID: ${{ secrets.COUPON_CI_CHAT_ID }}
+      run: |
+        HTTP_CODE=$(curl -s -o "$RUNNER_TEMP/getchat.json" -w "%{http_code}" \
+          "https://api.telegram.org/bot${BOT_TOKEN}/getChat?chat_id=${CHAT_ID}")
+        OK=$(jq -r '.ok // "null"' "$RUNNER_TEMP/getchat.json" 2>/dev/null || echo "null")
+        TYPE=$(jq -r '.result.type // ""' "$RUNNER_TEMP/getchat.json" 2>/dev/null || echo "")
+        TITLE=$(jq -r '.result.title // ""' "$RUNNER_TEMP/getchat.json" 2>/dev/null || echo "")
+        # Deliberately NOT printing .result.id here — for a group/supergroup
+        # getChat's returned id is the same secret value as COUPON_CI_CHAT_ID
+        # (or its supergroup-migrated form), so echoing it would leak the
+        # secret into the run log.
+        echo "getChat: http_code=$HTTP_CODE ok=$OK type=$TYPE title=$TITLE"
+
+        if [ "$HTTP_CODE" != "200" ] || [ "$OK" != "true" ]; then
+          echo "::error::getChat failed for COUPON_CI_CHAT_ID (http_code=$HTTP_CODE ok=$OK) — the chat id is wrong or the bot is not a member of that chat. Most likely cause: the group was upgraded from a basic group to a supergroup, which changes its id (Telegram assigns a new -100... id on upgrade — dev-bot-settings.sql's stale COMMUNITY_CHAT_ID is a precedent for this exact drift). Remediation: send any message in the group, call https://api.telegram.org/bot<token>/getUpdates and read the CURRENT chat.id off a recent update (or migrate_to_chat_id on the old chat's entry), then re-set the secret with: printf '%s' '<id>' | gh secret set COUPON_CI_CHAT_ID --repo Szer/bots (printf, not echo)."
+          exit 1
+        fi
+
+        if [ "$TYPE" = "group" ]; then
+          echo "::warning::COUPON_CI_CHAT_ID resolves to a basic 'group' (title=$TITLE), not a 'supergroup'. Basic-group ids are UNSTABLE — Telegram silently upgrades a group to a supergroup and its id changes shape when it does, breaking getChatMember-based membership checks until the secret is manually re-set. This is a standing risk, not a one-time fix."
+        fi
+
+        COUNT_HTTP_CODE=$(curl -s -o "$RUNNER_TEMP/getchatmembercount.json" -w "%{http_code}" \
+          "https://api.telegram.org/bot${BOT_TOKEN}/getChatMemberCount?chat_id=${CHAT_ID}")
+        COUNT_OK=$(jq -r '.ok // "null"' "$RUNNER_TEMP/getchatmembercount.json" 2>/dev/null || echo "null")
+        COUNT=$(jq -r '.result // ""' "$RUNNER_TEMP/getchatmembercount.json" 2>/dev/null || echo "")
+        echo "getChatMemberCount: http_code=$COUNT_HTTP_CODE ok=$COUNT_OK count=$COUNT"
+
+        if [ "$COUNT_HTTP_CODE" != "200" ] || [ "$COUNT_OK" != "true" ]; then
+          echo "::error::getChatMemberCount failed (http_code=$COUNT_HTTP_CODE ok=$COUNT_OK) for a chat that getChat above reported as valid — investigate manually before proceeding."
+          exit 1
+        fi
+
     - uses: actions/setup-dotnet@v4
       with:
         global-json-file: global.json
@@ -195,7 +285,18 @@ jobs:
             ('REMINDER_RUN_ON_START',  'false',                'FEATURE_FLAG', 'reminders',   'Reminders are driven on demand via /test/run-reminder instead'),
             ('REMINDER_HOUR_DUBLIN',   '10',                   'FREE_FORM',    'reminders',   'Irrelevant given REMINDER_RUN_ON_START=false; set for parity with prod'),
             ('BATCH_DEBOUNCE_MS',      '1000',                 'FREE_FORM',    'coupons',     'Short, so album/batch tests do not idle 5s each')
-        ON CONFLICT (key) DO NOTHING;
+        -- DO UPDATE, not DO NOTHING: migration V16__pending_add_batch.sql
+        -- already seeds BATCH_DEBOUNCE_MS='5000' with its own ON CONFLICT DO
+        -- NOTHING, so a fresh DB (via Flyway migrate above) already has that
+        -- row when this INSERT runs. DO NOTHING here would silently keep the
+        -- migration's 5000ms default instead of this step's CI-authoritative
+        -- 1000ms override — exactly the album/batch idling this row's own
+        -- description says it exists to avoid. DO UPDATE makes every value in
+        -- this list authoritative for the CI run regardless of what any
+        -- migration pre-seeds, matching every other row's existing intent
+        -- (see e.g. TEST_MODE's "must already be true at FIRST boot" comment
+        -- above).
+        ON CONFLICT (key) DO UPDATE SET value = EXCLUDED.value;
         SQL
 
         docker run --rm --network host \
@@ -291,8 +392,12 @@ jobs:
         INFO_LAST_ERROR=$(jq -r '.result.last_error_message // ""' "$RUNNER_TEMP/webhookinfo.json")
         echo "getWebhookInfo: http=$HTTP_CODE ok=$INFO_OK url=$INFO_URL last_error_message=$INFO_LAST_ERROR"
 
-        if [ "$HTTP_CODE" != "200" ] || [ "$INFO_OK" != "true" ] || [ "$INFO_URL" != "$EXPECTED_URL" ]; then
-          echo "::error::Webhook self-registration not confirmed — expected url '$EXPECTED_URL', got '$INFO_URL' (http=$HTTP_CODE ok=$INFO_OK). The bot likely did not self-register: verify WEBHOOK_URL reached the pod via the coupon-bot-env Secret and check the startup log with 'kubectl logs deployment/coupon-bot -n $NAMESPACE'."
+        if [ "$HTTP_CODE" != "200" ] || [ "$INFO_OK" != "true" ]; then
+          echo "::error::getWebhookInfo returned http=$HTTP_CODE ok=$INFO_OK — this shape (non-200 or ok=false) means Telegram rejected the BOT TOKEN itself, not that the webhook is unregistered (an unregistered webhook on a valid token returns http=200, ok=true, url=\"\"). See the 'Validate COUPON_CI_BOT_TOKEN shape and identity' step earlier in this run for the diagnosis (token shape/whitespace + getMe result)."
+          exit 1
+        fi
+        if [ "$INFO_URL" != "$EXPECTED_URL" ]; then
+          echo "::error::Webhook self-registration not confirmed — expected url '$EXPECTED_URL', got '$INFO_URL' (http=200, ok=true, so the token itself is valid). This means self-registration genuinely did not happen: verify WEBHOOK_URL reached the pod via the coupon-bot-env Secret and check the startup log with 'kubectl logs deployment/coupon-bot -n $NAMESPACE'."
           exit 1
         fi
         if [ -n "$INFO_LAST_ERROR" ]; then
@@ -353,6 +458,19 @@ jobs:
         path: test-artifacts/
         if-no-files-found: ignore
 
+    - name: Dump pod diagnostics on failure (before teardown deletes them)
+      # Teardown below runs with if: always() and deletes the deployment, so
+      # by the time a human reads the failed step's "run kubectl logs ..."
+      # advice, the pod is already gone. Capture it here first. Startup logs
+      # should not contain secrets (the webhook service only logs the URL),
+      # but redact-and-report rather than trust that blindly.
+      if: failure()
+      run: |
+        echo "--- kubectl -n $NAMESPACE get pods,deploy ---"
+        kubectl -n "$NAMESPACE" get pods,deploy || true
+        echo "--- kubectl -n $NAMESPACE logs deployment/coupon-bot --tail=200 ---"
+        kubectl -n "$NAMESPACE" logs deployment/coupon-bot --tail=200 || true
+
     - name: Teardown transient workload
       if: always()
       # Explicit kinds only — NOT `all` (kubectl expands that to every kind

src/CouponHubBot/Services/CouponFlowHandler.fs

@@ -96,7 +96,42 @@ type CouponFlowHandler(
             do! sendText chatId text
         }
 
-    member _.HandleAddManual (user: DbUser) (msg: Message) =
+    /// Best-effort OCR barcode lookup for the manual /add path. Only the barcode is
+    /// taken from OCR — value/min-check/expiry always come from the caption and are
+    /// never overridden — so manual adds gain barcode-based duplicate detection
+    /// without changing what the user typed. Gated on OCR_ENABLED like the other two
+    /// OCR call sites. Any failure (GetFile/download error, OCR backend error, no
+    /// barcode found) degrades to null exactly like today: adding a coupon must
+    /// never fail because OCR/Azure is unavailable.
+    member private _.TryOcrBarcodeForManualAdd (userId: int64) (photoFileId: string) : Task<string | null> =
+        task {
+            let ocrConfig = ocrOptions.Value
+            if not ocrConfig.OcrEnabled then
+                return null
+            else
+                try
+                    let! file = tg.CallExn(Funogram.Telegram.Req.GetFile.Make(photoFileId))
+                    let filePath = file.FilePath |> Option.defaultValue ""
+                    if String.IsNullOrWhiteSpace filePath then
+                        return null
+                    else
+                        let! bytes = tg.DownloadFile filePath
+                        if int64 bytes.Length > ocrConfig.OcrMaxFileSizeBytes then
+                            return null
+                        else
+                            let! ocr = couponOcr.Recognize(ReadOnlyMemory<byte>(bytes))
+                            let barcodeText =
+                                if String.IsNullOrWhiteSpace ocr.barcode then null else ocr.barcode
+                            logger.LogInformation(
+                                "Manual-add OCR result for user {UserId}: hasBarcode={HasBarcode}",
+                                userId, not (isNull barcodeText))
+                            return barcodeText
+                with ex ->
+                    logger.LogWarning(ex, "Manual-add OCR failed for user {UserId}; adding coupon without barcode", userId)
+                    return null
+        }
+
+    member this.HandleAddManual (user: DbUser) (msg: Message) =
         task {
             use a = botActivity.StartActivity("handleAdd")
             %a.SetTag("userId", user.id)
@@ -122,7 +157,8 @@ type CouponFlowHandler(
                     let largestPhoto =
                         photos
                         |> Array.maxBy (fun p -> p.FileSize |> Option.defaultValue 0L)
-                    match! db.TryAddCoupon(user.id, largestPhoto.FileId, value, minCheck, expiresAt, null) with
+                    let! barcodeText = this.TryOcrBarcodeForManualAdd user.id largestPhoto.FileId
+                    match! db.TryAddCoupon(user.id, largestPhoto.FileId, value, minCheck, expiresAt, barcodeText) with
                     | AddCouponResult.Added coupon ->
                         let v = coupon.value.ToString("0.##")
                         let mc = coupon.min_check.ToString("0.##")

tests/CouponHubBot.RealTests/BulkAddRealTests.fs

@@ -1,6 +1,7 @@
 namespace CouponHubBot.RealTests
 
 open System
+open System.Globalization
 open System.IO
 open System.Threading.Tasks
 open Dapper
@@ -11,14 +12,27 @@ open Xunit
 /// path in the bot."
 ///
 /// Uses 3 of the shared EXPIRED-coupon fixtures (same barcode-uniqueness/public-repo
-/// constraint as AddFlowRealTests — see its doc comment). Unlike the single-photo /add
-/// test, an album has no per-photo caption to override OCR's own (past) printed expiry
-/// with — CouponFlowHandler.fs's OcrItem/FinalizeBatch batch path has no expiry
-/// validation at all (only "did OCR find a barcode + value + min-check + date" gates
-/// 'ok' vs 'needs_input'; see CouponFlowHandler.fs:496-509), so this deliberately does
-/// NOT assert the resulting coupons are still available/unexpired — only that the
-/// stateful album -> debounce -> awaiting_user -> bulk-confirm -> DB pipeline completes
-/// for real, which is what "most stateful path in the bot" means to exercise.
+/// constraint as AddFlowRealTests — see its doc comment); their OCR-decoded barcodes
+/// (2706688198821 / …838 / …845) are distinct, verified both by reading the fixtures'
+/// raw Azure-OCR text and by the passing CouponHubBot.Ocr.Tests theory cases for these
+/// exact filenames, so no barcode-uniqueness collision is in play here.
+///
+/// Unlike the single-photo /add test, an album has no per-photo caption to override
+/// OCR's own (past) printed expiry with. CouponFlowHandler.fs's OcrItem gate (does OCR
+/// find a barcode + value + min-check + date; see CouponFlowHandler.fs:532-537) has no
+/// expiry check, but DbService.TryAddCoupon — called per-item from BulkBatchConfirm —
+/// DOES reject `expires_at < todayUtc()` (DbService.fs:198-200), same as the manual /add
+/// path. Since every fixture in Images/ is deliberately dated in the past (repo-public
+/// constraint above) and the album flow has no caption to carry an override, OCR's own
+/// printed expiry would always get every item silently skipped as "Expired" (0 net
+/// coupons added — this bit the suite for real: see git blame near this comment).
+/// `bumpItemsToFutureExpiry` below overrides just the batch items' `expires_at` via
+/// direct SQL after OCR lands (same "direct SQL, real-test-only" pattern
+/// ReminderRealTests.fs and AddFlowRealTests' caption both already use to get a
+/// real-but-photographed-expired fixture past TryAddCoupon's expiry gate) so the
+/// pipeline being exercised — album -> debounce -> awaiting_user -> bulk-confirm -> DB
+/// insert — completes for real and actually lands 3 coupons, which is what "most
+/// stateful path in the bot" and this test's own name both claim.
 ///
 /// BATCH_DEBOUNCE_MS is seeded to 1000ms (contract's bot_setting table) specifically so
 /// this test doesn't idle on the production 5s default — but it's still scheduled
@@ -96,6 +110,22 @@ type BulkAddRealTests(fx: RealAssemblyFixture) =
             return! conn.QuerySingleAsync<int64>("SELECT COUNT(*)::bigint FROM coupon WHERE owner_id=@u", {| u = ownerId |})
         }
 
+    /// The album flow has no caption to carry an expiry override, so — mirroring
+    /// AddFlowRealTests' explicit-future-caption trick via direct SQL instead — bump the
+    /// OCR-landed 'ok' items' expires_at past DbService.TryAddCoupon's `expires_at <
+    /// todayUtc()` gate (DbService.fs:198-200) before the batch is confirmed. Leaves
+    /// value/min_check/barcode_text (the actually-under-test OCR output) untouched.
+    let bumpItemsToFutureExpiry (batchId: int64) =
+        task {
+            let futureExpiry = DateTime.UtcNow.AddDays(400.).ToString("yyyy-MM-dd", CultureInfo.InvariantCulture)
+            use conn = new NpgsqlConnection(fx.DbConnectionString)
+            let! _ =
+                conn.ExecuteAsync(
+                    "UPDATE pending_add_batch_item SET expires_at=@e::date WHERE batch_id=@b AND status='ok'",
+                    {| b = batchId; e = futureExpiry |})
+            return ()
+        }
+
     [<Fact>]
     member _.``album of 3 photos finalizes into a bulk-confirm; confirming adds 3 coupons``() =
         TestRetry.withTimeoutRetry (fun () -> task {
@@ -109,6 +139,7 @@ type BulkAddRealTests(fx: RealAssemblyFixture) =
 
             let! batchId = waitForBatchByOwner ownerId
             do! waitForAllItemsTerminal batchId
+            do! bumpItemsToFutureExpiry batchId
             do! fx.AdvanceClockAsync 2000 // BATCH_DEBOUNCE_MS=1000 (contract seed) + margin
             do! waitForBatchStatus batchId "awaiting_user"