Merge branch 'master' into fix/storage-recycle-edge-gates

Author: mastermanas805

internal/handlers/brevo_webhook.go

@@ -356,10 +356,18 @@ func (h *BrevoTransactionalWebhookHandler) Receive(c *fiber.Ctx) error {
 		// B13-F7 / B18 wave-3: hydrate the canonical ErrorResponse envelope
 		// (ok/error/message/request_id/retry_after_seconds/agent_action) so
 		// schema validators on the wire see the same 4xx shape every other
-		// handler emits. respondError reads the canonical agent_action from
-		// codeToAgentAction["unauthorized"], so we get a consistent UX
-		// surface without a per-call override.
-		return respondError(c, fiber.StatusUnauthorized, "unauthorized",
+		// handler emits.
+		//
+		// API-6 (QA 2026-05-29): use the Brevo-specific error code
+		// `brevo_secret_mismatch` instead of the generic `unauthorized` so
+		// the canonical agent_action correctly tells the OPERATOR to fix
+		// their Brevo dashboard config, instead of telling a USER to log
+		// in for a new INSTANODE_TOKEN (this webhook is unrelated to user
+		// auth). HTTP status stays 401 — only the error CODE + agent_action
+		// copy change, so existing operator alerting that pivots off
+		// `metrics.BrevoWebhookEventsTotal{result="unauthorized"}` is
+		// unaffected.
+		return respondError(c, fiber.StatusUnauthorized, "brevo_secret_mismatch",
 			"Brevo webhook URL secret did not match the configured value.")
 	}
 

internal/handlers/brevo_webhook_test.go

@@ -30,8 +30,10 @@ package handlers_test
 import (
 	"bytes"
 	"errors"
+	"io"
 	"net/http"
 	"net/http/httptest"
+	"strings"
 	"testing"
 
 	sqlmock "github.com/DATA-DOG/go-sqlmock"
@@ -171,6 +173,47 @@ func TestBrevoTxWebhook_SecretMismatch_Returns401(t *testing.T) {
 	}
 }
 
+// TestBrevoTxWebhook_SecretMismatch_AgentActionMentionsBrevo pins API-6 (QA
+// 2026-05-29): the 401 envelope must carry an OPERATOR-targeted agent_action
+// telling whoever called this endpoint to verify their Brevo webhook URL,
+// NOT the generic "tell the user to log in for a new INSTANODE_TOKEN" copy
+// that ships on the canonical `unauthorized` error class. The Brevo webhook
+// is unrelated to user auth.
+func TestBrevoTxWebhook_SecretMismatch_AgentActionMentionsBrevo(t *testing.T) {
+	db, _, _ := sqlmock.New()
+	defer db.Close()
+	h := handlers.NewBrevoTransactionalWebhookHandler(db, &config.Config{BrevoWebhookSecret: testBrevoTxSecret})
+	app := brevoTxApp(t, h)
+
+	resp := postBrevoTx(t, app, "bogus-secret", `{"event":"delivered","message-id":"x"}`)
+	if resp.StatusCode != http.StatusUnauthorized {
+		t.Fatalf("status = %d; want 401", resp.StatusCode)
+	}
+
+	raw, err := io.ReadAll(resp.Body)
+	if err != nil {
+		t.Fatalf("read body: %v", err)
+	}
+	body := string(raw)
+
+	// Error CODE must be the Brevo-specific one — not the generic
+	// "unauthorized" that ships with the user-login agent_action.
+	if !strings.Contains(body, `"error":"brevo_secret_mismatch"`) {
+		t.Errorf("body must carry error=brevo_secret_mismatch; got %s", body)
+	}
+	// Agent action must mention Brevo / BREVO_WEBHOOK_SECRET — NOT
+	// "INSTANODE_TOKEN" or the generic login-recovery script.
+	if !strings.Contains(strings.ToLower(body), "brevo") {
+		t.Errorf("agent_action must mention Brevo; got %s", body)
+	}
+	if strings.Contains(body, "INSTANODE_TOKEN") {
+		t.Errorf("agent_action must NOT mention INSTANODE_TOKEN (unrelated to this webhook); got %s", body)
+	}
+	if strings.Contains(body, "log in at https://instanode.dev/login to mint a new one") {
+		t.Errorf("agent_action must NOT carry the user-login recovery script; got %s", body)
+	}
+}
+
 // ── 4. Closed-by-default: empty configured secret OR empty URL param
 
 func TestBrevoTxWebhook_EmptyConfiguredSecret_Returns401(t *testing.T) {

internal/handlers/helpers.go

@@ -145,6 +145,16 @@ var codeToAgentAction = map[string]errorCodeMeta{
 	"unauthorized": {
 		AgentAction: "Tell the user their INSTANODE_TOKEN is missing or invalid. Have them log in at https://instanode.dev/login to mint a new one — takes 30 seconds.",
 	},
+	// brevo_secret_mismatch is a Brevo webhook URL-path-token mismatch — NOT a
+	// user-auth failure. The generic "unauthorized" agent_action ("log in to mint
+	// a new INSTANODE_TOKEN") sent an unrelated recovery script that was
+	// uselessly wrong for the actual incident (operator must verify their Brevo
+	// dashboard webhook URL contains the configured BREVO_WEBHOOK_SECRET).
+	// API-6 (QA 2026-05-29): give this error its own copy. Follows the U3
+	// contract — "Tell the user" opening, https://instanode.dev/ URL, < 280 chars.
+	"brevo_secret_mismatch": {
+		AgentAction: "Tell the user this is a Brevo-webhook config mismatch, not their auth. Operators must verify the Brevo dashboard webhook URL matches the configured BREVO_WEBHOOK_SECRET — see https://instanode.dev/docs/email.",
+	},
 	"auth_required": {
 		AgentAction: "Tell the user this action requires an authenticated session. Have them log in or sign up at https://instanode.dev/login — both flows mint a token.",
 	},

internal/handlers/onboarding.go

@@ -37,42 +37,38 @@ func NewOnboardingHandler(db *sql.DB, cfg *config.Config, emailClient *email.Cli
 	return &OnboardingHandler{db: db, cfg: cfg, email: emailClient}
 }
 
-// StartLanding handles GET /start?t={jwt}
-// Validates the JWT and redirects to the dashboard ClaimPage.
+// StartLanding handles GET /start?t={jwt}.
+//
+// API-5 (QA 2026-05-29): per CLAUDE.md "Live API surface", /start must ALWAYS
+// 302 to the dashboard `/claim?t=jwt` — the dashboard is the user-facing
+// landing page that renders any token error (expired / unrecognised /
+// already-claimed) in a friendly UI. Previously, an invalid token surfaced
+// the raw `{"ok":false,"error":"invalid_token"}` JSON envelope with HTTP 400,
+// which is what an upgrade link printed in an agent's terminal log lands on
+// when copy-pasted into a browser — the user sees naked JSON, not a recovery
+// flow.
+//
+// The new contract: pass the token through verbatim and let the dashboard's
+// ClaimPage handle validation. Bonus: the platform side avoids a DB lookup on
+// every drive-by /start hit (the JTI lookup now happens once, at /claim time,
+// where it's actually load-bearing).
+//
+// Edge cases:
+//   - Missing `t` query: 302 to /claim (no token) — the dashboard's ClaimPage
+//     renders its "no token" empty state.
+//   - Token shape is preserved (url.QueryEscape on the raw value); no
+//     validation, no decoding — invalidity is the dashboard's concern.
+//
+// The landing-viewed metric still increments so the funnel pivot of
+// "agents that surface /start in their tool output" stays measurable.
 func (h *OnboardingHandler) StartLanding(c *fiber.Ctx) error {
-	ctx := c.UserContext()
-	requestID := middleware.GetRequestID(c)
 	jwtStr := c.Query("t")
-	if jwtStr == "" {
-		return respondError(c, fiber.StatusBadRequest, "missing_token", "Upgrade token is required")
-	}
-
-	claims, err := crypto.VerifyOnboardingJWT([]byte(h.cfg.JWTSecret), jwtStr)
-	if err != nil {
-		slog.Warn("onboarding.start.invalid_jwt",
-			"error", err,
-			"request_id", requestID,
-		)
-		return respondError(c, fiber.StatusBadRequest, "invalid_token", "Upgrade token is invalid or expired")
-	}
-
-	// Verify JTI exists and hasn't been converted.
-	ev, err := models.GetOnboardingByJTI(ctx, h.db, claims.ID)
-	if err != nil {
-		var notFound *models.ErrOnboardingNotFound
-		if errors.As(err, &notFound) {
-			return respondError(c, fiber.StatusBadRequest, "invalid_token", "Upgrade token not recognized")
-		}
-		slog.Error("onboarding.start.db_error", "error", err, "request_id", requestID)
-		return respondError(c, fiber.StatusServiceUnavailable, "lookup_failed", "Failed to verify upgrade token")
-	}
-
-	if ev.ConvertedAt.Valid {
-		return c.Redirect(h.cfg.DashboardBaseURL+"/claim?already_claimed=true", fiber.StatusFound)
-	}
 
 	metrics.ConversionFunnel.WithLabelValues("landing_viewed").Inc()
 
+	if jwtStr == "" {
+		return c.Redirect(h.cfg.DashboardBaseURL+"/claim", fiber.StatusFound)
+	}
 	return c.Redirect(h.cfg.DashboardBaseURL+"/claim?t="+url.QueryEscape(jwtStr), fiber.StatusFound)
 }
 

internal/handlers/onboarding_coverage_test.go

@@ -455,6 +455,13 @@ func TestClaim_HappyPath_FreshTeamAndUser(t *testing.T) {
 	assert.NotEmpty(t, got["session_token"])
 }
 
+// API-5 (QA 2026-05-29): /start ALWAYS 302s to the dashboard /claim, regardless
+// of token validity. The dashboard renders any error UI (expired / unrecognised
+// / already-claimed). Pre-fix, these tests asserted 400 JSON; the new contract
+// is "always-bounce", so they assert 302 to /claim instead. The
+// `already_claimed` flag no longer surfaces in the redirect URL because we no
+// longer look up the JTI at /start time — the dashboard does it.
+
 func TestStartLanding_AlreadyClaimedRedirectsToDashboardWithFlag(t *testing.T) {
 	db, clean := testhelpers.SetupTestDB(t)
 	defer clean()
@@ -489,10 +496,13 @@ func TestStartLanding_AlreadyClaimedRedirectsToDashboardWithFlag(t *testing.T) {
 	defer resp.Body.Close()
 	assert.Equal(t, http.StatusFound, resp.StatusCode)
 	loc := resp.Header.Get("Location")
-	assert.Contains(t, loc, "already_claimed=true")
+	// Always-302 contract: the dashboard handles already-claimed in its UI;
+	// the platform side just forwards the token verbatim.
+	assert.Contains(t, loc, "/claim?t=")
 }
 
 func TestStartLanding_MissingTokenReturns400(t *testing.T) {
+	// API-5: kept name for grep; new contract is 302 to /claim with no t= query.
 	db, clean := testhelpers.SetupTestDB(t)
 	defer clean()
 	rdb, cleanRedis := testhelpers.SetupTestRedis(t)
@@ -504,13 +514,15 @@ func TestStartLanding_MissingTokenReturns400(t *testing.T) {
 	resp, err := app.Test(req, 5000)
 	require.NoError(t, err)
 	defer resp.Body.Close()
-	assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
-	var body map[string]any
-	testhelpers.DecodeJSON(t, resp, &body)
-	assert.Equal(t, "missing_token", body["error"])
+	assert.Equal(t, http.StatusFound, resp.StatusCode)
+	loc := resp.Header.Get("Location")
+	assert.Contains(t, loc, "/claim")
+	assert.NotContains(t, loc, "/claim?t=", "missing token must redirect without t= query")
 }
 
 func TestStartLanding_UnknownJTI_400(t *testing.T) {
+	// API-5: kept name for grep; new contract is 302 to /claim?t=<jwt> — the
+	// dashboard does the JTI lookup and renders any error UI.
 	db, clean := testhelpers.SetupTestDB(t)
 	defer clean()
 	rdb, cleanRedis := testhelpers.SetupTestRedis(t)
@@ -527,7 +539,8 @@ func TestStartLanding_UnknownJTI_400(t *testing.T) {
 	resp, err := app.Test(req, 5000)
 	require.NoError(t, err)
 	defer resp.Body.Close()
-	assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
+	assert.Equal(t, http.StatusFound, resp.StatusCode)
+	assert.Contains(t, resp.Header.Get("Location"), "/claim?t=")
 }
 
 // ===== Email validation helpers =====

internal/handlers/onboarding_residual_test.go

@@ -95,61 +95,52 @@ func doGet(t *testing.T, app *fiber.App, path string) *http.Response {
 }
 
 // ── StartLanding ─────────────────────────────────────────────────────────────
-
-func TestResidualStartLanding_MissingToken_400(t *testing.T) {
+//
+// API-5 (QA 2026-05-29): /start now ALWAYS 302s to the dashboard /claim
+// regardless of token validity — the dashboard ClaimPage renders any token
+// error (expired / unrecognised / already-claimed) in a friendly UI. The
+// platform side no longer validates the JWT at /start; that's the dashboard's
+// job. Per CLAUDE.md "Live API surface" line.
+
+// TestResidualStartLanding_MissingToken_RedirectsToClaim — no token → 302 to
+// /claim (no t=) so the dashboard renders its empty / login state.
+func TestResidualStartLanding_MissingToken_RedirectsToClaim(t *testing.T) {
 	db, clean := testhelpers.SetupTestDB(t)
 	defer clean()
 	app := onboardingResidualApp(t, db)
 	resp := doGet(t, app, "/start")
-	assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
+	assert.Equal(t, http.StatusFound, resp.StatusCode)
+	assert.Contains(t, resp.Header.Get("Location"), "/claim")
+	// No t= query when missing.
+	assert.NotContains(t, resp.Header.Get("Location"), "/claim?t=")
 }
 
-func TestResidualStartLanding_InvalidJWT_400(t *testing.T) {
+// TestResidualStartLanding_GarbageToken_StillRedirects — invalid/garbage tokens
+// must NOT 400 the user with raw JSON. The dashboard renders the error.
+func TestResidualStartLanding_GarbageToken_StillRedirects(t *testing.T) {
 	db, clean := testhelpers.SetupTestDB(t)
 	defer clean()
 	app := onboardingResidualApp(t, db)
 	resp := doGet(t, app, "/start?t=garbage")
-	assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
+	assert.Equal(t, http.StatusFound, resp.StatusCode)
+	assert.Contains(t, resp.Header.Get("Location"), "/claim?t=garbage")
 }
 
-func TestResidualStartLanding_UnknownJTI_400(t *testing.T) {
+// TestResidualStartLanding_UnknownJTI_StillRedirects — a syntactically valid
+// JWT for an unknown JTI must also 302 — the dashboard does the JTI lookup
+// and renders the "expired/unrecognised" message.
+func TestResidualStartLanding_UnknownJTI_StillRedirects(t *testing.T) {
 	db, clean := testhelpers.SetupTestDB(t)
 	defer clean()
 	app := onboardingResidualApp(t, db)
 	signed := mintOnboardingJWT(t, uuid.NewString(), "fp-start-unknown", nil)
 	resp := doGet(t, app, "/start?t="+signed)
-	assert.Equal(t, http.StatusBadRequest, resp.StatusCode)
-}
-
-// TestStartLanding_DBError_503 drives the db_error arm (66-67) via a brokenDB:
-// JWT verifies in-process, then GetOnboardingByJTI errors with a non-notfound
-// error → 503 lookup_failed.
-func TestResidualStartLanding_DBError_503(t *testing.T) {
-	app := onboardingResidualApp(t, brokenDB(t))
-	signed := mintOnboardingJWT(t, uuid.NewString(), "fp-start-broken", nil)
-	resp := doGet(t, app, "/start?t="+signed)
-	assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode)
-}
-
-// TestStartLanding_AlreadyClaimed_Redirects drives the converted-redirect arm
-// (70-72): a converted onboarding row → 302 to the dashboard with the flag.
-func TestResidualStartLanding_AlreadyClaimed_Redirects(t *testing.T) {
-	db, clean := testhelpers.SetupTestDB(t)
-	defer clean()
-	app := onboardingResidualApp(t, db)
-	jti := uuid.NewString()
-	_, err := db.ExecContext(context.Background(), `
-		INSERT INTO onboarding_events (jti, fingerprint, converted_at, team_id)
-		VALUES ($1, $2, now(), NULL)
-	`, jti, "fp-start-claimed")
-	require.NoError(t, err)
-	signed := mintOnboardingJWT(t, jti, "fp-start-claimed", nil)
-	resp := doGet(t, app, "/start?t="+signed)
 	assert.Equal(t, http.StatusFound, resp.StatusCode)
-	assert.Contains(t, resp.Header.Get("Location"), "already_claimed=true")
+	assert.Contains(t, resp.Header.Get("Location"), "/claim?t=")
 }
 
-// TestStartLanding_HappyPath_Redirects drives the success redirect (74-76).
+// TestResidualStartLanding_HappyPath_Redirects — happy path is still a 302
+// with t= query intact. Same shape as the always-302 contract.
 func TestResidualStartLanding_HappyPath_Redirects(t *testing.T) {
 	db, clean := testhelpers.SetupTestDB(t)
 	defer clean()