feat: seller account assessment v2 — SSE streaming, 3-state eligibility, discovery UI

[alastrat]

Summary

Complete rewrite of the seller account assessment flow with real-time streaming and 3-state eligibility classification.

• Per-tenant SP-API credentials — each seller connects their own Amazon account via encrypted credentials (AES-256-GCM)
• 3-phase discovery assessment — broad category search (20 categories × 20 products), batch brand enrichment via LookupByIdentifier, eligibility checking with circuit breakers (6 safety mechanisms)
• 3-state eligibility classification — Eligible (can list now), Ungatable (APPROVAL_REQUIRED — can apply via Seller Central), Restricted (no path forward). Parses SP-API reasonCode and links to distinguish
• SSE real-time streaming — in-memory pub/sub hub (AssessmentHub) streams product discoveries to the browser via Server-Sent Events. Products appear on the radial tree graph one-by-one during scanning
• ECharts radial tree — hierarchical visualization: Root → Categories → Subcategories → Brands, color-coded by eligibility status (green/amber/red)
• Redesigned reveal step — stat cards (eligible/can-apply/restricted/categories), filterable product table with status tabs (All/Eligible/Can Apply/Restricted), approval links to Seller Central
• Onboarding wizard — 4-step flow (Connect → Discover → Reveal → Commit) with auto-skip, pause/resume, rescan, eligible-only toggle, CSV download

Key fixes

• SP-API brandName vs brand field name discrepancy (keyword search vs identifier lookup)
• Eligibility status persisted in DB (migration 019) — was being lost between SSE stream and fingerprint read
• Auto-advance bug — consolidated competing useEffect hooks into SSE-primary + API-fallback
• Browse node search returning 400 → switched to keyword search
• Inngest Docker connectivity (INNGEST_SERVE_HOST)

Test plan

• Connect Amazon seller account with SP-API credentials
• Trigger assessment scan — verify SSE stream in DevTools Network tab
• Verify products appear incrementally on radial tree during scan
• Verify 3-state badges: green (Eligible), amber (Apply + Request link), red (Restricted)
• Click "Request" link on ungatable product → opens Seller Central approval page
• Filter tabs work: All/Eligible/Can Apply/Restricted
• Rescan button resets and starts fresh scan
• Eligible Only toggle filters tree to green+amber nodes only
• go test ./internal/... passes (263 tests)
• npx tsc --noEmit passes

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Connect and manage Amazon SP‑API seller accounts to validate credentials and start assessments.
  • Real‑time discovery with streaming progress, interactive radial opportunity graph, and deduplicated product table with CSV export.
  • Assessment outcomes now include qualified product recommendations or an “Ungating Roadmap”.

• UI/UX Updates

  • Onboarding updated to emphasize account connection, auto‑advance when connected, rescan, and conditional commit actions.

• Tests

  • New Playwright end‑to‑end onboarding and smoke tests.

• Chores

  • Local start/stop scripts and Makefile targets for dev, testing, and deploy workflows.

• Documentation

  • New design/implementation plans and strategy/risk docs added.

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds encrypted seller-account storage and verification, a discovery-driven assessment pipeline with SSE via a new AssessmentHub, DB migrations and repos for enriched probe/product fields, API routes for seller accounts and assessment graph/events, frontend ECharts discovery graph/product table with SSE hooks, Playwright E2E tests, and start/stop scripts/Makefile targets.

Changes

Cohort / File(s)	Summary
Makefile & Scripts Makefile, scripts/start.sh, scripts/stop.sh	Add start/stop targets and scripts; local convenience flow; Playwright test/install targets; assess and web-deploy helpers.
API Startup & Config apps/api/main.go, internal/config/config.go	Wire AES encryptor, AmazonSellerAccount repo/service, and AssessmentHub; load ENCRYPTION_KEY; conditional test-account seeding at startup.
Router & Handlers internal/api/router.go, internal/api/handler/assessment_handler.go, internal/api/handler/seller_account_handler.go	Register /seller-account endpoints and new assessment endpoints /assessment/graph and /assessment/events; assessment handler now uses hub; new SellerAccount handler (connect/get/disconnect).
Domain & Ports internal/domain/*.go, internal/port/*.go	Add AmazonSellerAccount model and status enum; expand seller_profile types for discovery/outcomes; add ScanTypeAssessment; new SellerAccountRepo interface; add EligibilityStatus and extend ListingRestriction.
Crypto & Encryptor internal/domain/crypto.go	Add AESEncryptor (AES-256-GCM) with dev passthrough and Encrypt/Decrypt APIs.
Postgres Repo & Migrations internal/adapter/postgres/..., internal/adapter/postgres/migrations/*	New AmazonSellerAccount repo that encrypts secrets; probe results expanded (title/price/est_margin_pct/seller_count/subcategory/eligibility_status/approval_url); migrations for seller accounts, product fields, subcategory, eligibility_status, seed migration, and RLS force.
Inngest Runtime & Triggering internal/adapter/inngest/client.go	Simplify TriggerAssessment to tenant-only; runtime switches to RunDiscoveryAssessment and fails early if per-tenant SP-API client absent.
SP-API Adapter internal/adapter/spapi/client.go	Add NewClientFromCredentials; improve brand extraction and diagnostics; richer eligibility parsing (status, reasonCode, approvalURL).
Assessment Hub & Service internal/service/assessment_hub.go, internal/service/assessment_service.go, internal/service/assessment_service_test.go	Add AssessmentHub (per-tenant pub/sub with bounded history); refactor assessment service to RunDiscoveryAssessment (three-phase discovery/enrichment/outcome), centralized SSE publishing; update tests and mocks accordingly.
Seller Account Service internal/service/seller_account_service.go	New service to connect/verify/disconnect accounts, persist status, and build per-tenant SP-API clients.
Frontend: API client, types, keys apps/web/src/lib/api-client.ts, apps/web/src/lib/types.ts, apps/web/src/lib/query-keys.ts, apps/web/package.json	API client adds token accessor and seller-account endpoints; relax startAssessment signature; add getAssessmentGraph; new types for SSE/tree/outcome/products; add echarts + echarts-for-react.
Frontend hooks & SSE apps/web/src/hooks/use-assessment.ts, apps/web/src/hooks/use-assessment-sse.ts	Add hooks for connect/get/disconnect seller account and assessment graph; new useAssessmentSSE EventSource consumer with batching and tree construction.
Frontend components & pages apps/web/src/components/discovery-graph.tsx, apps/web/src/components/discovery-product-table.tsx, apps/web/src/app/(app)/onboarding/page.tsx, apps/web/src/app/(app)/dashboard/page.tsx	Add DiscoveryGraph (ECharts radial tree) and DiscoveryProductTable (filtering, CSV export); onboarding flow reworked to connect→discover→reveal→commit using SSE and graph/product table; small dashboard copy edit.
E2E Tests & Docs apps/web/tests/e2e/onboarding.spec.ts, docs/superpowers/*.md, docs/strategy/*.md	Add Playwright onboarding E2E tests and multiple planning/spec docs for assessment UI, ECharts radial tree, enrichment, go-to-market, risks, and agent orchestration patterns.

Sequence Diagram(s)

sequenceDiagram
    actor User
    participant Web as Browser
    participant API as API Server
    participant Hub as Assessment Hub
    participant SPAPI as Amazon SP-API
    participant DB as Database

    User->>Web: Submit SP-API credentials
    Web->>API: POST /seller-account/connect
    API->>SPAPI: CheckListingEligibility (credential verify)
    alt valid
        SPAPI-->>API: eligibility OK
        API->>DB: Encrypt & upsert account
        API-->>Web: 200 {connected:true, account}
    else invalid
        SPAPI-->>API: error
        API->>DB: upsert status/error
        API-->>Web: 400/500 {error}
    end

    User->>Web: Start assessment
    Web->>API: POST /assessment/start
    API->>Hub: StartStream(tenant)
    API->>API: RunDiscoveryAssessment (per-category loop)
    loop per category
        API->>SPAPI: SearchByBrowseNode / SearchProducts
        SPAPI-->>API: products
        API->>SPAPI: CheckListingEligibility / GetProductDetails
        SPAPI-->>API: eligibility + details
        API->>Hub: Publish(category_start/product_found/category_complete)
        Hub-->>Web: SSE events
    end
    API->>DB: persist fingerprint & probe results
    API->>Hub: EndStream(tenant)
    Hub-->>Web: SSE "done"
    Web->>API: GET /assessment/graph
    API->>DB: load tree/products/stats
    API-->>Web: {tree, products, stats}
    Web->>Web: Render DiscoveryGraph + DiscoveryProductTable

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

• feat: AI concierge — shared catalog, credits, assessment, strategy, suggestions (Phases 0-4) #2: Closely overlaps assessment SSE, AssessmentHub, seller-account repo/service/handler, DB migrations, and frontend onboarding/streaming.
• feat: high-volume discovery engine (Phases A-F) #1: Related runtime/inngest discovery changes and assessment trigger wiring.

Poem

🐰 I nibbled keys and hopped through code,
I hid secrets safe in AES mode.
Radial trees spun, products took their place,
Streams of discoveries danced through space.
Tiny hops became a grander chase. 🌿

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 31.34% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: seller account assessment v2 with SSE streaming, 3-state eligibility, and discovery UI, which aligns with the comprehensive refactor across backend assessment flow and frontend onboarding UI.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/seller-account-assessment-v2

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 20

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)

internal/adapter/inngest/client.go (1)

957-980: ⚠️ Potential issue | 🟡 Minor

Clarify whether missing fingerprint after discovery should fail the workflow or silently skip strategy generation.

RunDiscoveryAssessment persists the fingerprint with warning-level error handling (line 202 in assessment_service.go):

if err := s.persistFingerprint(ctx, tenantID, allResults, categoryStats); err != nil {
    slog.Warn("assessment: failed to persist fingerprint", "error", err)
}

If fingerprint persistence fails (e.g., database issue), the discovery still completes successfully, but later when GetFingerprint is called in the strategy generation step (line 960-964), it will return nil/error and the code silently skips strategy generation with a warning and returns no error. This leaves the assessment outcome without a corresponding strategy.

Either:

1. Fingerprint persistence should be required (fail discovery if it fails), or
2. The silent strategy skip should explicitly signal to the caller that strategy was skipped

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/adapter/inngest/client.go` around lines 957 - 980, The code silently
skips strategy generation when assessmentSvc.GetFingerprint returns nil/error,
which can hide upstream persistFingerprint failures; update behavior to either
surface the persistence failure or explicitly signal a skipped strategy: modify
persistFingerprint in RunDiscoveryAssessment/assessment_service.go to return an
error (remove or change the slog.Warn-only handling) so discovery fails when
fingerprint persistence fails, and/or change the strategy generation block in
client.go (inside the step.Run "build-strategy") to treat a nil fp or non-nil
err from assessmentSvc.GetFingerprint as a hard error (return that error instead
of returning "", nil) and log a clear message referencing
GenerateInitialStrategy, GetFingerprint, strategySvc and assessmentSvc so
callers can detect that strategy generation was skipped due to missing
fingerprint.

internal/api/handler/assessment_handler.go (1)

35-49: ⚠️ Potential issue | 🟠 Major

Avoid leaving assessments stuck in running when the workflow trigger fails.

StartAssessment persists the profile before the async discovery job is enqueued. If TriggerAssessment returns an error, this handler responds with 500 but the tenant already has a stored assessment in running, with nothing scheduled to advance or fail it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/api/handler/assessment_handler.go` around lines 35 - 49, The handler
currently calls h.assessment.StartAssessment(...) which persists a profile as
"running" before calling h.durableRuntime.TriggerAssessment(...); if
TriggerAssessment fails you must rollback or mark that persisted assessment as
failed to avoid leaving it stuck. After TriggerAssessment returns an error, call
the assessment service method that updates the persisted profile status (e.g., a
delete or SetStatus/MarkFailed/FailAssessment function on h.assessment) to set
the assessment to a terminal failed state (include any error message), log the
rollback action and any rollback error, and then return the original 500
response; ensure you reference StartAssessment, TriggerAssessment and
h.durableRuntime/h.assessment when locating code to change.

🟡 Minor comments (6)

docs/superpowers/specs/2026-04-10-subcategory-brand-enrichment.md-183-187 (1)

183-187: ⚠️ Potential issue | 🟡 Minor

Clarify sequencing: eligible_ASINs cannot be known before eligibility checks.

Line 183 references eligible_ASINs before the eligibility pass on Line 186. Please reword to avoid contradictory execution order.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/superpowers/specs/2026-04-10-subcategory-brand-enrichment.md` around
lines 183 - 187, The spec currently references eligible_ASINs before running
eligibility checks; change the wording so CheckListingEligibility runs first to
produce eligible_ASINs, then call Batch GetProductDetails(eligible_ASINs, 20 per
batch) to fetch real price and seller_count and merge buy_box_price and
seller_count into product data; specifically update the sequence around the
symbols CheckListingEligibility and Batch GetProductDetails so eligibility is
determined prior to batching product detail requests.

docs/superpowers/plans/2026-04-10-assessment-ui-fixes.md-30-35 (1)

30-35: ⚠️ Potential issue | 🟡 Minor

Align the hierarchy here with the 4-level model used elsewhere.

Line 30–35 skips the subcategory layer, which conflicts with the current tree model (Root → Category → Subcategory → Brand). This can cause implementation drift for follow-up fixes.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/superpowers/plans/2026-04-10-assessment-ui-fixes.md` around lines 30 -
35, Update the described hierarchy to match the canonical 4-level tree model
used elsewhere: include the missing Subcategory layer between Category and Brand
so the visualization becomes Level 0: Amazon Marketplace (root) → Level 1:
Category (click to expand) → Level 2: Subcategory (click to expand) → Level 3:
Brand (click to expand) → Level 4: Products (drill-in), and adjust any
references to "Level 1–3" in the description to reflect the new Level 0–4 naming
to avoid implementation drift.

apps/web/src/components/discovery-graph.tsx-239-240 (1)

239-240: ⚠️ Potential issue | 🟡 Minor

Legend label is inconsistent with actual subcategory coloring.

Line 239–240 says subcategory is gray, but subcategory nodes are dynamically color-coded by ratios. The legend text/color should match runtime behavior.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/discovery-graph.tsx` around lines 239 - 240, The
legend shows a static gray swatch but subcategory nodes are color-coded at
runtime; update the legend in discovery-graph.tsx to use the same colorization
logic as the subcategory nodes instead of the hardcoded "bg-gray-400" span —
call/reuse the existing color function or component used for rendering nodes
(e.g., getColorForRatio, colorScale, or the SubcategoryNode color helper) to
compute a sample color (or render the same swatch component) and replace the
static span so the legend color matches runtime coloring; keep the label
"Subcategory (colored by eligible ratio)" as-is.

apps/web/src/app/(app)/onboarding/page.tsx-91-104 (1)

91-104: ⚠️ Potential issue | 🟡 Minor

Fix authentication header in handleRescan.

The Authorization header logic at line 97 appears inverted and uses document.cookie.includes() incorrectly. If a token cookie exists, it sets an empty bearer token, which will fail authentication.

Consider using apiClient for consistency with the rest of the codebase, or fix the header logic:

🔧 Option 1: Use apiClient (preferred)

+import { apiClient } from "@/lib/api-client";

 function handleRescan() {
-  fetch(
-    `${process.env.NEXT_PUBLIC_API_URL || "http://localhost:8081"}/assessment/reset`,
-    {
-      method: "DELETE",
-      headers: { Authorization: `Bearer ${document.cookie.includes("token") ? "" : "dev-user-dev-tenant"}` },
-    },
-  ).then(() => {
+  apiClient.resetAssessment().then(() => {
     startAssessment.mutate(undefined, {
       onSuccess: () => changeStep("discover"),
     });
   });
 }

This requires adding a resetAssessment method to api-client.ts if it doesn't exist.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/app/`(app)/onboarding/page.tsx around lines 91 - 104, The
Authorization header in handleRescan is inverted and sends an empty Bearer when
a token exists; update handleRescan to send the actual token when present (e.g.,
read the token cookie or auth store and set Authorization: `Bearer <token>`), or
preferably replace the manual fetch with the shared apiClient by adding and
calling a resetAssessment method in api-client.ts and invoking that from
handleRescan; keep the existing startAssessment.mutate(...) and
changeStep("discover") flow intact after the apiClient/reset succeeds.

apps/web/src/hooks/use-assessment-sse.ts-191-203 (1)

191-203: ⚠️ Potential issue | 🟡 Minor

Missing est_margin_pct extraction from SSE event data.

The backend sends est_margin_pct in the product_found event payload (per internal/service/assessment_service.go:445-460), but the catchup handler hardcodes it to 0 at line 198. The same issue exists in the PRODUCT_BATCH handler at line 249.

🔧 Proposed fix

           products.push({
             asin,
             title: (d.title as string) || "",
             brand: (d.brand as string) || "Generic",
             category: (d.category as string) || "",
             subcategory: (d.subcategory as string) || "",
             price: (d.price as number) || 0,
-            est_margin_pct: 0,
+            est_margin_pct: (d.est_margin_pct as number) || 0,
             seller_count: (d.seller_count as number) || 0,

Apply the same fix at line 249 in the PRODUCT_BATCH handler.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/hooks/use-assessment-sse.ts` around lines 191 - 203, The
est_margin_pct is being hardcoded to 0 in the products.push payload in the
product_found handler (and likewise in the PRODUCT_BATCH handler); update the
products.push creation in use-assessment-sse.ts to pull est_margin_pct from the
SSE event data (e.g., use (d.est_margin_pct as number) || 0) instead of the
constant 0, and apply the identical change in the PRODUCT_BATCH handler so both
handlers extract and populate est_margin_pct from the incoming event payload.

apps/web/src/lib/types.ts-300-320 (1)

300-320: ⚠️ Potential issue | 🟡 Minor

AmazonSellerAccount doesn't match the seller-account API payloads.

internal/api/handler/seller_account_handler.go omits sp_api_client_id and updated_at from both Connect/Get responses, and GET adds a connected flag that this interface doesn't model. Consumers typed as AmazonSellerAccount will end up trusting fields that are undefined at runtime.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/lib/types.ts` around lines 300 - 320, The AmazonSellerAccount
type is out of sync with the backend: the API responses do not include
sp_api_client_id or updated_at but do include a connected flag; update the
AmazonSellerAccount interface to match the API by removing (or at least making
optional) sp_api_client_id and updated_at and adding connected?: boolean, and
then adjust any consumers typed as AmazonSellerAccount to accept the changed
shape (look for references to AmazonSellerAccount and
ConnectSellerAccountRequest to ensure types align with
internal/api/handler/seller_account_handler.go).

🧹 Nitpick comments (9)

internal/adapter/spapi/client.go (2)

192-194: Consider extracting brand fallback to a helper function.

The brand extraction logic (brandName → brand fallback) is duplicated in three places: SearchProducts, SearchByBrowseNode, and LookupByIdentifier. A small helper would reduce repetition.

♻️ Optional helper extraction

// extractBrand tries brandName first, then falls back to brand.
func extractBrand(summary map[string]any) string {
	if b, _ := summary["brandName"].(string); b != "" {
		return b
	}
	b, _ := summary["brand"].(string)
	return b
}

Then use p.Brand = extractBrand(s) in each location.

Also applies to: 313-315, 711-715

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/adapter/spapi/client.go` around lines 192 - 194, Duplicate brand
fallback logic (checking "brandName" then "brand") is present in SearchProducts,
SearchByBrowseNode, and LookupByIdentifier; extract that into a small helper
(e.g., func extractBrand(summary map[string]any) string) that returns the
non-empty "brandName" or falls back to "brand", then replace the inline blocks
setting p.Brand with p.Brand = extractBrand(s) (referencing the existing
variable s used in those functions) to remove repetition.

---

671-694: Consider reducing log verbosity for production.

The diagnostic logging at Info level (lines 686-692) including raw JSON summaries may be verbose in production. Consider using slog.Debug for the raw summary output, keeping only aggregate counts at Info level.

♻️ Suggested log level adjustment

-			slog.Info("sp-api: identifier lookup brand check", "total", len(items), "with_brand", brandsFound)
+			slog.Debug("sp-api: identifier lookup brand check", "total", len(items), "with_brand", brandsFound)
 			// Log first item's raw summaries for debugging
 			if first, ok := items[0].(map[string]any); ok {
 				if sums, ok := first["summaries"].([]any); ok && len(sums) > 0 {
 					sumJSON, _ := json.Marshal(sums[0])
-					slog.Info("sp-api: first item summaries", "raw", string(sumJSON))
+					slog.Debug("sp-api: first item summaries", "raw", string(sumJSON))
 				}
 			}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/adapter/spapi/client.go` around lines 671 - 694, The diagnostic
block that computes brandsFound should keep the aggregate count logging at Info
but reduce verbosity by changing the raw-first-item summary log from slog.Info
to slog.Debug; specifically update the call that logs "sp-api: first item
summaries" (and any associated raw JSON output creation for sums[0]) to use
slog.Debug instead of slog.Info so raw JSON is only emitted in debug mode while
leaving the aggregate slog.Info("sp-api: identifier lookup brand check", ...)
intact.

scripts/stop.sh (2)

22-25: Same quoting fix needed for Next.js PID.

🔧 Proposed fix

 if [ -f /tmp/fba-web.pid ]; then
-  kill $(cat /tmp/fba-web.pid) 2>/dev/null && echo "  Stopped Next.js" || true
+  kill "$(cat /tmp/fba-web.pid)" 2>/dev/null && echo "  Stopped Next.js" || true
   rm -f /tmp/fba-web.pid
 fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/stop.sh` around lines 22 - 25, The kill invocation that reads the
Next.js PID from /tmp/fba-web.pid should be made safe by quoting and using the
POSIX-safe end-of-options marker; change the unquoted substitution used with
kill to use a quoted command substitution and a "--" option (i.e. replace the
kill $(cat /tmp/fba-web.pid) usage) so PIDs containing unexpected characters or
leading dashes won't be misinterpreted; keep the existing 2>/dev/null && echo " 
Stopped Next.js" || true flow and the subsequent rm -f /tmp/fba-web.pid step.

---

15-18: Quote command substitution to prevent word splitting.

Shellcheck correctly flags that $(cat /tmp/fba-api.pid) should be quoted to handle potential edge cases with whitespace or special characters in the PID file.

🔧 Proposed fix

 if [ -f /tmp/fba-api.pid ]; then
-  kill $(cat /tmp/fba-api.pid) 2>/dev/null && echo "  Stopped Go API" || true
+  kill "$(cat /tmp/fba-api.pid)" 2>/dev/null && echo "  Stopped Go API" || true
   rm -f /tmp/fba-api.pid
 fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/stop.sh` around lines 15 - 18, The kill invocation uses unquoted
command substitution for the PID file (/tmp/fba-api.pid); update the kill line
so the command substitution is quoted (i.e., quote the $(cat /tmp/fba-api.pid)
used in the kill command) to prevent word-splitting or globbing when reading the
PID; keep the existing conditional and pid-file removal logic (the rm -f
/tmp/fba-api.pid line can remain unchanged).

apps/web/src/app/(app)/onboarding/page.tsx (2)

155-166: Potential stale closure in auto-advance effect.

The changeStep function is recreated on every render but isn't included in the dependency array. While this works because changeStep only uses setSelectedNode and setStep (which have stable identities), it's fragile and could cause issues if changeStep logic changes.

💡 Wrap changeStep in useCallback

+import { useState, useEffect, useCallback } from "react";

-  function changeStep(next: Step) {
-    setSelectedNode(null);
-    setStep(next);
-  }
+  const changeStep = useCallback((next: Step) => {
+    setSelectedNode(null);
+    setStep(next);
+  }, []);

Then add changeStep to both useEffect dependency arrays.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/app/`(app)/onboarding/page.tsx around lines 155 - 166, Wrap the
changeStep function in useCallback to give it a stable identity (e.g., const
changeStep = useCallback((s) => { ... }, [setSelectedNode, setStep]) or include
any other internal deps), then add changeStep to the dependency array of the
auto-advance useEffect that currently depends on [step, sse.isComplete,
sse.connected, graphData?.status]; also update any other useEffect that uses
changeStep to include it in their dependency arrays so the effect won’t capture
a stale closure.

---

73-87: Effect may trigger redundant assessment starts.

The effect at lines 74-87 checks sellerAccount?.status === "valid" and starts an assessment if none is running/completed. However, startAssessment.mutate isn't included in the dependency array (correctly), but if the effect re-runs while a mutation is in flight, it could trigger duplicate requests.

Consider adding a guard:

   useEffect(() => {
-    if (sellerAccount?.status === "valid" && step === "connect") {
+    if (sellerAccount?.status === "valid" && step === "connect" && !startAssessment.isPending) {
       if (assessment?.status === "completed") {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/app/`(app)/onboarding/page.tsx around lines 73 - 87, The effect
that auto-starts an assessment (the useEffect watching sellerAccount,
assessment, step) can call startAssessment.mutate multiple times if re-run while
a mutation is in flight; add a guard that checks the mutation state (e.g.
startAssessment.isLoading or a local ref like isStarting.current) before calling
startAssessment.mutate so you only initiate the mutation when not already
loading, and ensure you set/reset the ref or rely on startAssessment's
onSuccess/onError to prevent duplicate starts while the mutation is pending;
keep changeStep and the existing assessment.status checks unchanged.

Makefile (1)

82-84: Consider parameterizing the production API URL.

The hardcoded https://amazonagent-production.up.railway.app URL in web-deploy couples the Makefile to a specific deployment. Consider using an environment variable for flexibility.

 # Deploy frontend to Cloudflare
 web-deploy:
-	cd apps/web && NEXT_PUBLIC_API_URL=https://amazonagent-production.up.railway.app npm run deploy
+	cd apps/web && NEXT_PUBLIC_API_URL=$${API_URL:-https://amazonagent-production.up.railway.app} npm run deploy

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Makefile` around lines 82 - 84, Update the web-deploy Makefile target to stop
hardcoding the production API URL: make the NEXT_PUBLIC_API_URL value taken from
an environment variable (e.g., PROD_API_URL) with an optional default fallback
so callers can override it; edit the web-deploy target that currently sets
NEXT_PUBLIC_API_URL to the fixed https://amazonagent-production.up.railway.app
so it uses the env var (or default) instead.

apps/web/tests/e2e/onboarding.spec.ts (1)

82-135: Mock timing issue may cause test flakiness.

The test navigates to /onboarding at line 84-85 in beforeEach, but the mocks are only set up after the page loads (lines 87-126). The initial page load won't use the mocked responses, so the page won't auto-advance to the Discover step.

The second goto("/onboarding") at line 130 will use the mocks, but by then the test description ("shows progress stats and graph during scanning") may not reflect what's actually being tested.

Consider restructuring to set up routes before navigation:

 test.describe("Onboarding — Discover step with graph", () => {
-  test("shows progress stats and graph during scanning", async ({ page }) => {
-    await page.goto("/onboarding");
-
-    // Mock: already connected + assessment running
+  test("shows progress stats and graph during scanning", async ({ page }) => {
+    // Set up mocks BEFORE navigation
     await page.route("**/seller-account", async (route) => {
       // ...
     });
+    // ... other routes ...
+
+    await page.goto("/onboarding");
+
+    // Now verify the expected UI

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/tests/e2e/onboarding.spec.ts` around lines 82 - 135, The mocks are
registered after the first page.goto which causes flakiness; move the route
setup (the page.route handlers for "**/seller-account", "**/assessment/status",
and "**/assessment/graph") to run before navigating to "/onboarding" (i.e.,
before the first page.goto) or remove the initial goto and only call
page.goto("/onboarding") after registering those routes so the Discover step
loads with mocked responses; locate the route setup and page.goto calls in the
test "shows progress stats and graph during scanning" to reorder them
accordingly.

internal/domain/seller_profile.go (1)

133-137: Enforce AssessmentOutcome state invariants.

HasOpportunities, Opportunity, and Ungating can currently conflict. Add validation or constructors so exactly one branch is present and consistent.

🔧 Proposed fix

 type AssessmentOutcome struct {
 	HasOpportunities bool               `json:"has_opportunities"`
 	Opportunity      *OpportunityResult `json:"opportunity,omitempty"`
 	Ungating         *UngatingResult    `json:"ungating,omitempty"`
@@
 }
+
+func (o AssessmentOutcome) Validate() error {
+	if o.HasOpportunities {
+		if o.Opportunity == nil || o.Ungating != nil {
+			return fmt.Errorf("invalid opportunity outcome shape")
+		}
+		return nil
+	}
+	if o.Opportunity != nil || o.Ungating == nil {
+		return fmt.Errorf("invalid ungating outcome shape")
+	}
+	return nil
+}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/domain/seller_profile.go` around lines 133 - 137, Add constructors
and/or a validation method to enforce that AssessmentOutcome is in exactly one
state: either an opportunity branch or an ungating branch (not both, and not
neither), and ensure HasOpportunities is consistent with the presence of
Opportunity. Implement two constructors like
NewAssessmentOutcomeWithOpportunity(op *OpportunityResult) and
NewAssessmentOutcomeWithUngating(u *UngatingResult) that populate fields
appropriately (set HasOpportunities true only for opportunity constructor) and
make the struct's Validate() method check the invariant (exactly one of
Opportunity or Ungating non-nil and HasOpportunities matches). Replace any
direct struct literals/creations in code with these constructors or call
Validate() after creation to prevent inconsistent states.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/web/src/components/discovery-graph.tsx`:
- Around line 145-148: The brand tooltip currently checks only nd.eligible and
collapses the three-state model; update the tooltip generation in the block
where type === "brand" (the branch using nd and d.name) to read
nd.eligibility_status instead of nd.eligible and map its values to the correct
labels (e.g., "Eligible", "Restricted", "Ungatable") so the returned string
preserves all three states in the tooltip text.
- Around line 142-151: Escape all interpolated tooltip values before inserting
into the HTML formatter: replace direct uses of d.name and nd.asin with an
HTML-escaped version (use or add a small escapeHtml function that replaces
&<>"'` with entities) in the formatter used for tooltips in discovery-graph.tsx
so ECharts cannot render injected markup; also stop using the boolean
nd.eligible and instead read nd.eligibility_status (values "eligible" |
"ungatable" | "restricted") and render that full three-state label (escaped) in
the brand tooltip so the badge shows the correct status.

In `@apps/web/src/components/discovery-product-table.tsx`:
- Around line 69-83: The CSV export currently builds rows directly and
inconsistently escapes cells; to harden against formula injection and malformed
cells, create a sanitizeCell helper used in rows mapping (referencing
displayed.map and rows) that: converts any value to string, doubles internal
quotes, and if the string begins with any of the dangerous leading characters
(=, +, -, @) prefixes it with a single quote; then wrap every field in double
quotes (even numbers and URLs) so values like price, est_margin_pct,
seller_count, getStatus(p) and p.approval_url are serialized via sanitizeCell
before joining into csv, keeping headers as-is and leaving blob creation
unchanged.
- Around line 23-29: The component returns early before later hooks (useMemo,
useCallback) run, violating Rules of Hooks; to fix, ensure all hooks in
DiscoveryProductTable run on every render by removing the early return and
instead compute a renderBody (or isEmpty placeholder) after executing the hooks,
or move the conditional render to a JSX branch that uses a variable (e.g.,
showPlaceholder) so that useMemo and useCallback (the hooks referenced) are
always invoked in the same order inside the DiscoveryProductTable component;
update the JSX to render the dashed placeholder when !selectedNode &&
!showAllByDefault without skipping hook execution.

In `@apps/web/src/lib/types.ts`:
- Around line 324-360: The TypeScript type AssessmentGraph does not match the
actual response from GetGraph (which returns { status, tree, products, stats });
update the type definitions to match that shape: replace the top-level keys
nodes/edges with status: string (or union), tree: appropriate node type (e.g.,
AssessmentGraphNode tree structure), products: product array type, and make
stats fields (like restricted_brands) optional or remove fields that the Go
handler never returns; ensure AssessmentGraphNode/AssessmentGraphStats names
(and any consumers) are adjusted to reference the new properties so callers
don't read undefined values.

In `@internal/adapter/postgres/migrations/015_amazon_seller_accounts.sql`:
- Around line 23-25: Add FORCE ROW LEVEL SECURITY to the amazon_seller_accounts
table to prevent owner-level policy bypass: modify the ALTER TABLE statement for
amazon_seller_accounts (the row level security enabling line) so the table
enforces RLS for all roles, ensuring the amazon_seller_accounts_isolation policy
(USING (tenant_id = current_setting('app.tenant_id', true)::uuid)) cannot be
bypassed by table owners or superusers.

In `@internal/api/handler/assessment_handler.go`:
- Around line 90-400: The handler GetGraph currently contains heavy business
logic (canonicalizing categories, aggregating BrandResults, building the tree
and computing stats); move this into the assessment service by adding a method
on the AssessmentService (e.g., AssessmentService.GetAssessmentGraph or
BuildAssessmentGraph) that accepts context and tenantID (and any needed inputs
like fingerprint/profile) and returns a DTO containing status, tree, products,
and stats; then change AssessmentHandler.GetGraph to only extract auth
(middleware.GetAuthContext), call the new service method, handle errors, and
JSON-encode the returned DTO (preserving existing field names). Ensure you
remove the aggregation code from the handler (unique symbols: GetGraph,
AssessmentHandler, fingerprint.BrandResults, fingerprint.Categories) and add
unit tests for the new service method to validate equivalence with the previous
output.

In `@internal/domain/crypto.go`:
- Around line 23-27: The NewAESEncryptor function currently silently enables
plaintext devMode when keyHex is empty; change its signature to
NewAESEncryptor(keyHex string, allowPlaintext bool) (or similar) and make it
return an error when keyHex is empty and allowPlaintext is false, only setting
AESEncryptor{devMode:true} when allowPlaintext is true; update all call sites
(e.g. where NewAESEncryptor is invoked in apps/api/main.go) to pass cfg.Env ==
"development" (or equivalent) as the allowPlaintext flag so production will fail
fast if ENCRYPTION_KEY is missing.

In `@internal/domain/seller_profile.go`:
- Around line 86-93: Remove the redundant Eligible bool field and consolidate
eligibility into a single canonical EligibilityStatus value: update the
struct(s) BrandProbeResult and AssessmentSearchResult (and the seller_profile.go
struct) to drop the Eligible bool and use a typed EligibilityStatus constant
enum instead; add a helper like statusFromBool(eligible bool) to convert
existing boolean assignments (replace usages of Eligible: eligible) to set
EligibilityStatus accordingly, and update all assignment sites and JSON handling
to only read/write EligibilityStatus so the frontend fallback is no longer
needed.
- Around line 96-97: Fields representing money in the domain model (e.g., Price
and EstMarginPct) must not be float64; change them to a decimal-safe type (for
example shopspring/decimal.Decimal or an int64 representing cents) and update
all callers that do arithmetic — notably any code using p.AmazonPrice (e.g., the
wholesaleCost := p.AmazonPrice * 0.4 calculation) and the estimated monthly
revenue computation that performs chained multiplications — to use the chosen
decimal API or integer-cent math; leave non-currency fields like DurationSeconds
as-is and ensure JSON marshal/unmarshal and DB NUMERIC(10,2) conversions are
adjusted accordingly.

In `@internal/port/tools.go`:
- Around line 46-63: Tests are constructing ListingRestriction without setting
the non-omitempty Status field, leaving it as the empty string; update each test
location that builds a ListingRestriction to set Status to a valid
EligibilityStatus constant (e.g., Status: port.EligibilityEligible or
port.EligibilityUngatable / port.EligibilityRestricted as appropriate for the
test scenario) so the struct matches the production initialization in
ListingRestriction and avoids invalid empty status values.

In `@internal/service/assessment_hub.go`:
- Around line 92-99: The delayed cleanup goroutine unconditionally deletes
h.streams[tenantID] after 30s and can remove a newly created stream; fix by
capturing the current tenantStream reference before sleeping (e.g., orig :=
h.streams[tenantID]) and after the sleep re-locking and checking that
h.streams[tenantID] == orig (or is nil) before calling delete; update the
anonymous goroutine in EndStream (or wherever the goroutine is launched) to
perform this identity check using h.mu, tenantID and tenantStream to avoid
deleting a replaced stream.

In `@internal/service/assessment_service.go`:
- Around line 182-183: filterEligible(...) currently includes
approval-required/ungatable items, so phase3BuildOutcome's TotalEligible (using
eligibleResults := filterEligible(allResults)) double-counts products also
reported in TotalUngatable; change TotalEligible to count only immediately
sellable items by excluding ungatable/approval-required results (e.g., compute
sellableResults := filterSellable(allResults) or derive sellable by filtering
eligibleResults where IsUngatable/RequiresApproval is false) and use that for
TotalEligible; update the same logic in the other occurrences (the blocks
referenced around phase3BuildOutcome and the other ranges) so TotalEligible and
TotalUngatable are mutually exclusive.
- Around line 352-355: The code currently hardcodes the "US" marketplace in
discovery/enrichment calls (e.g., spapi.SearchProducts) which breaks per-tenant
marketplace support; change those calls to use the tenant's connected
marketplace variable instead of the literal "US" (locate where the
tenant/assessment context is available—e.g., Tenant.Marketplace,
tenant.ConnectedMarketplace, or the assessment/ctx wrapper used in this service)
and pass that marketplace value into spapi.SearchProducts and the other SP-API
calls referenced (same change for the other occurrences around lines 378-379,
408-409, 497-498) so all search, brand enrichment, eligibility checks, and
detail enrichment use the tenant-specific marketplace. Ensure cs.addAPICalls
usage remains unchanged.
- Around line 296-325: The jump logic after cs.fireBreaker is dead because
scanOrder already iterates from highest to lowest ExpectedOpen so the check
DiscoveryCategories[jumpIdx].ExpectedOpen > cat.ExpectedOpen always fails;
update the loop in assessment logic to pick the highest-remaining unscanned
category instead of requiring a strictly greater ExpectedOpen. Concretely, in
the block that iterates scanOrder (referencing scanOrder, DiscoveryCategories,
cat.ExpectedOpen, scanned, cs.consecutiveEmptyCats and s.scanOneCategory),
remove the > cat.ExpectedOpen condition and select the first unscanned jumpIdx
(the highest-remaining) to mark scanned and call s.scanOneCategory, preserving
the existing scanned[jumpIdx] = true, appending to allResults/catStats,
resetting cs.consecutiveEmptyCats when eligible > 0, and breaking after one
jump.

In `@internal/service/seller_account_service.go`:
- Around line 79-83: The current upsert deletes the existing account
unconditionally and ignores Delete errors, risking data loss if Create fails;
change the logic in the seller account service to perform an atomic upsert
instead: either call a repository method that does CreateOrUpdate/Upsert in a
transaction (preferred) or first try s.repo.Create(ctx, account) and if it
returns a "already exists" error call s.repo.Update(ctx, account); properly
handle and return errors from s.repo.Delete if you must keep the delete path,
and ensure the repository exposes/implements transactional Upsert semantics (or
use DB transaction in the repo) so s.repo.Delete and s.repo.Create are not
executed separately and irreversibly.
- Around line 45-56: The code currently uses marketplaceFromID to derive a
region code (e.g., "US") and passes that to SP-API calls, which breaks requests
that expect the real marketplace ID; replace uses of marketplaceFromID in SP-API
interactions so the real input.MarketplaceID is passed to SP-API clients and
methods (e.g., when constructing testClient via spapi.NewClientFromCredentials
and when calling testClient.CheckListingEligibility); keep marketplaceFromID
only for internal/UX mapping if needed, and update the other similar spots
referenced (the later usages around the other
CheckListingEligibility/marketplace call sites) to use input.MarketplaceID
instead.

In `@scripts/start.sh`:
- Around line 39-45: The readiness loops that poll "docker compose ps postgres"
(the for loop checking grep -q "healthy") and the similar web readiness loop
must fail fast on timeout: after each loop, check whether the loop exited via
success (found "healthy") or timed out and, if timed out, print a clear error
message and exit non‑zero (e.g., echo "Postgres not ready after timeout" && exit
1); do the same for the web readiness loop (the block around lines 93-104) so
the script does not continue to "All services running" when services never
became ready.
- Around line 64-65: The script exports the wrong environment variable name
(INNGEST_SERVE_HOST) which prevents Inngest callback registration; update the
export to use INNGEST_SERVE_ORIGIN instead (retain the same value
"http://host.docker.internal:8081") and remove or replace any other occurrences
of INNGEST_SERVE_HOST in the script so all references use INNGEST_SERVE_ORIGIN
consistently.

---

Outside diff comments:
In `@internal/adapter/inngest/client.go`:
- Around line 957-980: The code silently skips strategy generation when
assessmentSvc.GetFingerprint returns nil/error, which can hide upstream
persistFingerprint failures; update behavior to either surface the persistence
failure or explicitly signal a skipped strategy: modify persistFingerprint in
RunDiscoveryAssessment/assessment_service.go to return an error (remove or
change the slog.Warn-only handling) so discovery fails when fingerprint
persistence fails, and/or change the strategy generation block in client.go
(inside the step.Run "build-strategy") to treat a nil fp or non-nil err from
assessmentSvc.GetFingerprint as a hard error (return that error instead of
returning "", nil) and log a clear message referencing GenerateInitialStrategy,
GetFingerprint, strategySvc and assessmentSvc so callers can detect that
strategy generation was skipped due to missing fingerprint.

In `@internal/api/handler/assessment_handler.go`:
- Around line 35-49: The handler currently calls
h.assessment.StartAssessment(...) which persists a profile as "running" before
calling h.durableRuntime.TriggerAssessment(...); if TriggerAssessment fails you
must rollback or mark that persisted assessment as failed to avoid leaving it
stuck. After TriggerAssessment returns an error, call the assessment service
method that updates the persisted profile status (e.g., a delete or
SetStatus/MarkFailed/FailAssessment function on h.assessment) to set the
assessment to a terminal failed state (include any error message), log the
rollback action and any rollback error, and then return the original 500
response; ensure you reference StartAssessment, TriggerAssessment and
h.durableRuntime/h.assessment when locating code to change.

---

Minor comments:
In `@apps/web/src/app/`(app)/onboarding/page.tsx:
- Around line 91-104: The Authorization header in handleRescan is inverted and
sends an empty Bearer when a token exists; update handleRescan to send the
actual token when present (e.g., read the token cookie or auth store and set
Authorization: `Bearer <token>`), or preferably replace the manual fetch with
the shared apiClient by adding and calling a resetAssessment method in
api-client.ts and invoking that from handleRescan; keep the existing
startAssessment.mutate(...) and changeStep("discover") flow intact after the
apiClient/reset succeeds.

In `@apps/web/src/components/discovery-graph.tsx`:
- Around line 239-240: The legend shows a static gray swatch but subcategory
nodes are color-coded at runtime; update the legend in discovery-graph.tsx to
use the same colorization logic as the subcategory nodes instead of the
hardcoded "bg-gray-400" span — call/reuse the existing color function or
component used for rendering nodes (e.g., getColorForRatio, colorScale, or the
SubcategoryNode color helper) to compute a sample color (or render the same
swatch component) and replace the static span so the legend color matches
runtime coloring; keep the label "Subcategory (colored by eligible ratio)"
as-is.

In `@apps/web/src/hooks/use-assessment-sse.ts`:
- Around line 191-203: The est_margin_pct is being hardcoded to 0 in the
products.push payload in the product_found handler (and likewise in the
PRODUCT_BATCH handler); update the products.push creation in
use-assessment-sse.ts to pull est_margin_pct from the SSE event data (e.g., use
(d.est_margin_pct as number) || 0) instead of the constant 0, and apply the
identical change in the PRODUCT_BATCH handler so both handlers extract and
populate est_margin_pct from the incoming event payload.

In `@apps/web/src/lib/types.ts`:
- Around line 300-320: The AmazonSellerAccount type is out of sync with the
backend: the API responses do not include sp_api_client_id or updated_at but do
include a connected flag; update the AmazonSellerAccount interface to match the
API by removing (or at least making optional) sp_api_client_id and updated_at
and adding connected?: boolean, and then adjust any consumers typed as
AmazonSellerAccount to accept the changed shape (look for references to
AmazonSellerAccount and ConnectSellerAccountRequest to ensure types align with
internal/api/handler/seller_account_handler.go).

In `@docs/superpowers/plans/2026-04-10-assessment-ui-fixes.md`:
- Around line 30-35: Update the described hierarchy to match the canonical
4-level tree model used elsewhere: include the missing Subcategory layer between
Category and Brand so the visualization becomes Level 0: Amazon Marketplace
(root) → Level 1: Category (click to expand) → Level 2: Subcategory (click to
expand) → Level 3: Brand (click to expand) → Level 4: Products (drill-in), and
adjust any references to "Level 1–3" in the description to reflect the new Level
0–4 naming to avoid implementation drift.

In `@docs/superpowers/specs/2026-04-10-subcategory-brand-enrichment.md`:
- Around line 183-187: The spec currently references eligible_ASINs before
running eligibility checks; change the wording so CheckListingEligibility runs
first to produce eligible_ASINs, then call Batch
GetProductDetails(eligible_ASINs, 20 per batch) to fetch real price and
seller_count and merge buy_box_price and seller_count into product data;
specifically update the sequence around the symbols CheckListingEligibility and
Batch GetProductDetails so eligibility is determined prior to batching product
detail requests.

---

Nitpick comments:
In `@apps/web/src/app/`(app)/onboarding/page.tsx:
- Around line 155-166: Wrap the changeStep function in useCallback to give it a
stable identity (e.g., const changeStep = useCallback((s) => { ... },
[setSelectedNode, setStep]) or include any other internal deps), then add
changeStep to the dependency array of the auto-advance useEffect that currently
depends on [step, sse.isComplete, sse.connected, graphData?.status]; also update
any other useEffect that uses changeStep to include it in their dependency
arrays so the effect won’t capture a stale closure.
- Around line 73-87: The effect that auto-starts an assessment (the useEffect
watching sellerAccount, assessment, step) can call startAssessment.mutate
multiple times if re-run while a mutation is in flight; add a guard that checks
the mutation state (e.g. startAssessment.isLoading or a local ref like
isStarting.current) before calling startAssessment.mutate so you only initiate
the mutation when not already loading, and ensure you set/reset the ref or rely
on startAssessment's onSuccess/onError to prevent duplicate starts while the
mutation is pending; keep changeStep and the existing assessment.status checks
unchanged.

In `@apps/web/tests/e2e/onboarding.spec.ts`:
- Around line 82-135: The mocks are registered after the first page.goto which
causes flakiness; move the route setup (the page.route handlers for
"**/seller-account", "**/assessment/status", and "**/assessment/graph") to run
before navigating to "/onboarding" (i.e., before the first page.goto) or remove
the initial goto and only call page.goto("/onboarding") after registering those
routes so the Discover step loads with mocked responses; locate the route setup
and page.goto calls in the test "shows progress stats and graph during scanning"
to reorder them accordingly.

In `@internal/adapter/spapi/client.go`:
- Around line 192-194: Duplicate brand fallback logic (checking "brandName" then
"brand") is present in SearchProducts, SearchByBrowseNode, and
LookupByIdentifier; extract that into a small helper (e.g., func
extractBrand(summary map[string]any) string) that returns the non-empty
"brandName" or falls back to "brand", then replace the inline blocks setting
p.Brand with p.Brand = extractBrand(s) (referencing the existing variable s used
in those functions) to remove repetition.
- Around line 671-694: The diagnostic block that computes brandsFound should
keep the aggregate count logging at Info but reduce verbosity by changing the
raw-first-item summary log from slog.Info to slog.Debug; specifically update the
call that logs "sp-api: first item summaries" (and any associated raw JSON
output creation for sums[0]) to use slog.Debug instead of slog.Info so raw JSON
is only emitted in debug mode while leaving the aggregate slog.Info("sp-api:
identifier lookup brand check", ...) intact.

In `@internal/domain/seller_profile.go`:
- Around line 133-137: Add constructors and/or a validation method to enforce
that AssessmentOutcome is in exactly one state: either an opportunity branch or
an ungating branch (not both, and not neither), and ensure HasOpportunities is
consistent with the presence of Opportunity. Implement two constructors like
NewAssessmentOutcomeWithOpportunity(op *OpportunityResult) and
NewAssessmentOutcomeWithUngating(u *UngatingResult) that populate fields
appropriately (set HasOpportunities true only for opportunity constructor) and
make the struct's Validate() method check the invariant (exactly one of
Opportunity or Ungating non-nil and HasOpportunities matches). Replace any
direct struct literals/creations in code with these constructors or call
Validate() after creation to prevent inconsistent states.

In `@Makefile`:
- Around line 82-84: Update the web-deploy Makefile target to stop hardcoding
the production API URL: make the NEXT_PUBLIC_API_URL value taken from an
environment variable (e.g., PROD_API_URL) with an optional default fallback so
callers can override it; edit the web-deploy target that currently sets
NEXT_PUBLIC_API_URL to the fixed https://amazonagent-production.up.railway.app
so it uses the env var (or default) instead.

In `@scripts/stop.sh`:
- Around line 22-25: The kill invocation that reads the Next.js PID from
/tmp/fba-web.pid should be made safe by quoting and using the POSIX-safe
end-of-options marker; change the unquoted substitution used with kill to use a
quoted command substitution and a "--" option (i.e. replace the kill $(cat
/tmp/fba-web.pid) usage) so PIDs containing unexpected characters or leading
dashes won't be misinterpreted; keep the existing 2>/dev/null && echo "  Stopped
Next.js" || true flow and the subsequent rm -f /tmp/fba-web.pid step.
- Around line 15-18: The kill invocation uses unquoted command substitution for
the PID file (/tmp/fba-api.pid); update the kill line so the command
substitution is quoted (i.e., quote the $(cat /tmp/fba-api.pid) used in the kill
command) to prevent word-splitting or globbing when reading the PID; keep the
existing conditional and pid-file removal logic (the rm -f /tmp/fba-api.pid line
can remain unchanged).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: c1d9d6af-7fde-41c0-aa14-723a0f6e264f

📥 Commits

Reviewing files that changed from the base of the PR and between 2caf09b and c1df8a4.

⛔ Files ignored due to path filters (1)

• apps/web/package-lock.json is excluded by !**/package-lock.json

📒 Files selected for processing (43)

• Makefile
• apps/api/main.go
• apps/web/package.json
• apps/web/playwright-report/index.html
• apps/web/src/app/(app)/dashboard/page.tsx
• apps/web/src/app/(app)/onboarding/page.tsx
• apps/web/src/components/discovery-graph.tsx
• apps/web/src/components/discovery-product-table.tsx
• apps/web/src/hooks/use-assessment-sse.ts
• apps/web/src/hooks/use-assessment.ts
• apps/web/src/lib/api-client.ts
• apps/web/src/lib/query-keys.ts
• apps/web/src/lib/types.ts
• apps/web/tests/e2e/onboarding.spec.ts
• docs/superpowers/plans/2026-04-10-assessment-ui-fixes.md
• docs/superpowers/plans/2026-04-10-echarts-radial-tree.md
• docs/superpowers/specs/2026-04-10-subcategory-brand-enrichment.md
• internal/adapter/inngest/client.go
• internal/adapter/postgres/amazon_seller_account_repo.go
• internal/adapter/postgres/eligibility_fingerprint_repo.go
• internal/adapter/postgres/migrations/015_amazon_seller_accounts.sql
• internal/adapter/postgres/migrations/016_seed_test_seller_account.sql
• internal/adapter/postgres/migrations/017_probe_result_product_fields.sql
• internal/adapter/postgres/migrations/018_subcategory.sql
• internal/adapter/postgres/migrations/019_eligibility_status.sql
• internal/adapter/spapi/client.go
• internal/api/handler/assessment_handler.go
• internal/api/handler/seller_account_handler.go
• internal/api/middleware/auth.go
• internal/api/router.go
• internal/config/config.go
• internal/domain/amazon_seller_account.go
• internal/domain/catalog.go
• internal/domain/crypto.go
• internal/domain/seller_profile.go
• internal/port/seller_account.go
• internal/port/tools.go
• internal/service/assessment_hub.go
• internal/service/assessment_service.go
• internal/service/assessment_service_test.go
• internal/service/seller_account_service.go
• scripts/start.sh
• scripts/stop.sh

[coderabbitai]

⚠️ Potential issue | 🟠 Major

AssessmentGraph models a different shape than /assessment/graph returns.

GetGraph in internal/api/handler/assessment_handler.go returns { status, tree, products, stats }, but this interface expects { nodes, edges, stats } and requires restricted_brands, which the handler never sends. Any caller using AssessmentGraph for that endpoint will read undefined properties.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/lib/types.ts` around lines 324 - 360, The TypeScript type
AssessmentGraph does not match the actual response from GetGraph (which returns
{ status, tree, products, stats }); update the type definitions to match that
shape: replace the top-level keys nodes/edges with status: string (or union),
tree: appropriate node type (e.g., AssessmentGraphNode tree structure),
products: product array type, and make stats fields (like restricted_brands)
optional or remove fields that the Go handler never returns; ensure
AssessmentGraphNode/AssessmentGraphStats names (and any consumers) are adjusted
to reference the new properties so callers don't read undefined values.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Keep the real marketplace ID for SP-API calls.

marketplaceFromID collapses values like ATVPDKIKX0DER to "US" and silently maps unknown IDs to "US". But the provided internal/adapter/spapi/client.go:542-580 snippet uses the supplied value directly as the Catalog Restrictions marketplaceId= query parameter, so this mapping turns requests into marketplaceId=US and misroutes unsupported marketplaces.

Also applies to: 119-123, 127-137

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/seller_account_service.go` around lines 45 - 56, The code
currently uses marketplaceFromID to derive a region code (e.g., "US") and passes
that to SP-API calls, which breaks requests that expect the real marketplace ID;
replace uses of marketplaceFromID in SP-API interactions so the real
input.MarketplaceID is passed to SP-API clients and methods (e.g., when
constructing testClient via spapi.NewClientFromCredentials and when calling
testClient.CheckListingEligibility); keep marketplaceFromID only for internal/UX
mapping if needed, and update the other similar spots referenced (the later
usages around the other CheckListingEligibility/marketplace call sites) to use
input.MarketplaceID instead.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Fail fast when readiness loops time out.

If Postgres or web never becomes ready, the script still continues and prints “All services running”. Add explicit timeout failure checks after both loops.

Suggested fix

-echo "     Waiting for Postgres to be healthy..."
-for i in $(seq 1 30); do
+echo "     Waiting for Postgres to be healthy..."
+PG_READY=false
+for i in $(seq 1 30); do
   if docker compose ps postgres | grep -q "healthy"; then
     echo -e "     ${GREEN}Postgres ready.${NC}"
+    PG_READY=true
     break
   fi
   sleep 1
 done
+if [ "$PG_READY" != "true" ]; then
+  echo -e "     ${RED}Postgres failed to become healthy.${NC}"
+  exit 1
+fi
@@
-for i in $(seq 1 30); do
+WEB_READY=false
+for i in $(seq 1 30); do
   if curl -s http://localhost:3001 > /dev/null 2>&1; then
     echo -e "     ${GREEN}Frontend ready (PID: $WEB_PID).${NC}"
+    WEB_READY=true
     break
   fi
   sleep 1
 done
+if [ "$WEB_READY" != "true" ]; then
+  echo -e "     ${RED}Frontend failed to start. Check /tmp/fba-web.log${NC}"
+  exit 1
+fi

Also applies to: 93-104

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/start.sh` around lines 39 - 45, The readiness loops that poll "docker
compose ps postgres" (the for loop checking grep -q "healthy") and the similar
web readiness loop must fail fast on timeout: after each loop, check whether the
loop exited via success (found "healthy") or timed out and, if timed out, print
a clear error message and exit non‑zero (e.g., echo "Postgres not ready after
timeout" && exit 1); do the same for the web readiness loop (the block around
lines 93-104) so the script does not continue to "All services running" when
services never became ready.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify which env key is used across the repo and startup scripts
rg -n "INNGEST_SERVE_(HOST|ORIGIN)|inngest dev -u"

Repository: alastrat/amazonagent

Length of output: 384

---

Use the correct Inngest serve-origin env var for local callback registration.

Line 64 exports INNGEST_SERVE_HOST, but the codebase documentation specifies INNGEST_SERVE_ORIGIN should be used. This mismatch will break Inngest callback registration.

-export INNGEST_SERVE_HOST="http://host.docker.internal:8081"
+export INNGEST_SERVE_ORIGIN="http://host.docker.internal:8081"

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	export INNGEST_SERVE_HOST="http://host.docker.internal:8081"
	export OPENFANG_API_URL=""
	export INNGEST_SERVE_ORIGIN="http://host.docker.internal:8081"
	export OPENFANG_API_URL=""

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/start.sh` around lines 64 - 65, The script exports the wrong
environment variable name (INNGEST_SERVE_HOST) which prevents Inngest callback
registration; update the export to use INNGEST_SERVE_ORIGIN instead (retain the
same value "http://host.docker.internal:8081") and remove or replace any other
occurrences of INNGEST_SERVE_HOST in the script so all references use
INNGEST_SERVE_ORIGIN consistently.

[coderabbitai]

Actionable comments posted: 3

♻️ Duplicate comments (3)

apps/web/src/components/discovery-product-table.tsx (1)

67-70: ⚠️ Potential issue | 🟠 Major

Harden CSV formula guard against prefixed payloads.

Current regex blocks only direct leading = + - @. Spreadsheet payloads prefixed with whitespace/control chars can still evaluate (e.g. \t=...).

Suggested fix

   const csvCell = (value: unknown) => {
     const raw = String(value ?? "");
-    const guarded = /^[=+\-@]/.test(raw) ? `'${raw}` : raw;
+    const guarded = /^[\t\r\n ]*[=+\-@]/.test(raw) ? `'${raw}` : raw;
     return `"${guarded.replace(/"/g, '""')}"`;
   };

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/discovery-product-table.tsx` around lines 67 - 70,
The csvCell function's formula-guard currently checks only the first character
and misses payloads with leading whitespace/control chars (e.g. "\t=...");
update csvCell so it detects formula initiators after any leading
whitespace/control characters and prefixes a single quote immediately before the
formula character while preserving the original leading whitespace, then
continue to escape internal quotes and wrap in double quotes as before (operate
on variables raw and guarded inside csvCell and keep the final replace(/"/g,
'""') behavior).

internal/service/assessment_service.go (2)

354-355: ⚠️ Potential issue | 🟠 Major

Use tenant marketplace instead of hardcoded "US" in all SP-API calls.

These calls still pin discovery/enrichment/eligibility/detail checks to US, which breaks per-tenant marketplace behavior.

Suggested fix (thread marketplace through scan path)

 func (s *AssessmentService) RunDiscoveryAssessment(
 	ctx context.Context,
 	tenantID domain.TenantID,
+	marketplace string,
 	spapi port.ProductSearcher,
 ) (*domain.AssessmentOutcome, error) {
@@
-	allResults, categoryStats := s.phase1SearchCategories(ctx, tenantID, spapi, cs)
+	allResults, categoryStats := s.phase1SearchCategories(ctx, tenantID, marketplace, spapi, cs)

 func (s *AssessmentService) phase1SearchCategories(
 	ctx context.Context,
 	tenantID domain.TenantID,
+	marketplace string,
 	spapi port.ProductSearcher,
 	cs *circuitState,
 ) ([]domain.AssessmentSearchResult, []categoryStat) {
@@
-		catResult := s.scanOneCategory(ctx, tenantID, spapi, cat, cs)
+		catResult := s.scanOneCategory(ctx, tenantID, marketplace, spapi, cat, cs)

 func (s *AssessmentService) scanOneCategory(
 	ctx context.Context,
 	tenantID domain.TenantID,
+	marketplace string,
 	spapi port.ProductSearcher,
 	cat AssessmentCategory,
 	cs *circuitState,
 ) categorySearchResult {
@@
-	products, err := spapi.SearchProducts(ctx, []string{cat.Name}, "US")
+	products, err := spapi.SearchProducts(ctx, []string{cat.Name}, marketplace)
@@
-	enriched, err := spapi.LookupByIdentifier(ctx, asins, "ASIN", "US")
+	enriched, err := spapi.LookupByIdentifier(ctx, asins, "ASIN", marketplace)
@@
-	restrictions, err := spapi.CheckListingEligibility(ctx, []string{p.ASIN}, "US")
+	restrictions, err := spapi.CheckListingEligibility(ctx, []string{p.ASIN}, marketplace)
@@
-		enriched, err := spapi.GetProductDetails(ctx, eligibleASINs, "US")
+		enriched, err := spapi.GetProductDetails(ctx, eligibleASINs, marketplace)

Also applies to: 378-379, 408-409, 497-498

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/assessment_service.go` around lines 354 - 355, Several
SP-API calls (e.g., spapi.SearchProducts) are using a hardcoded "US"
marketplace; replace that literal with the tenant-specific marketplace value and
thread it through the scan flow. Locate calls to spapi.SearchProducts and the
other SP-API calls flagged (the surrounding code that also calls cs.addAPICalls)
and change the marketplace argument from "US" to the tenant marketplace variable
(for example tenant.Marketplace or a marketplace string passed into the scan
function), ensure the marketplace is passed down the call chain where the scan
is initiated so functions that call spapi.SearchProducts,
discovery/enrichment/eligibility/detail checks use that marketplace value
instead of "US".

---

300-325: ⚠️ Potential issue | 🟠 Major

Repeated-failure jump condition is still effectively unreachable.

At Line 303, requiring ExpectedOpen > cat.ExpectedOpen means no jump candidate is found once scanning is already in descending order.

Suggested fix

-			for _, jumpIdx := range scanOrder {
-				if !scanned[jumpIdx] && DiscoveryCategories[jumpIdx].ExpectedOpen > cat.ExpectedOpen {
+			for _, jumpIdx := range scanOrder {
+				if !scanned[jumpIdx] {
 					// Reorder: put this one next

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/assessment_service.go` around lines 300 - 325, The jump loop
never finds a candidate when scanning in descending ExpectedOpen because it
requires DiscoveryCategories[jumpIdx].ExpectedOpen > cat.ExpectedOpen; change
the condition to allow equal-rate jumps (e.g. >=) and ensure you don't jump to
the same category by also checking jumpIdx != current category index (or another
available identity for cat) so the repeated-failure jump path can be taken;
update the condition in the loop that picks jumpIdx (and keep existing
scanned[jumpIdx] and cs.shouldStop() checks) so scanOneCategory is actually
invoked for equal-rate categories and cs.consecutiveEmptyCats logic remains
correct.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/web/src/components/discovery-graph.tsx`:
- Around line 101-103: The product node coloring currently only checks
node.eligible in the switch's case "product" in discovery-graph.tsx, which
collapses the three states; update the color assignment to first test
node.ungatable (assign the ungatable color, e.g., a distinct color), then
node.eligible (keep "#86efac"), and otherwise the restricted color (e.g.,
"#fca5a5"), so the switch's case "product" distinguishes ungatable, eligible,
and restricted states correctly.

In `@internal/service/assessment_service.go`:
- Around line 255-267: The scan loop in the function iterating over scanOrder
only checks circuit-breaker methods (cs.shouldStop(), cs.budgetExhausted(),
cs.timeExceeded(), cs.earlySuccess()) and doesn't honor context cancellation;
update the loop to check the request context (e.g., cs.ctx or a passed-in ctx)
by testing ctx.Err() or selecting on ctx.Done() at the top of the loop and
immediately break/return when canceled so the scan stops promptly; ensure the
same context check is added alongside the existing checks (near the scanOrder
loop and also at the other referenced spot around lines 402-405) and use
existing methods like cs.fireBreaker only after detecting cancellation if you
still need to log a breaker event.
- Around line 378-399: The call count for spapi.LookupByIdentifier isn't always
recorded because cs.addAPICalls is only invoked when enriched is non-empty;
update the logic around spapi.LookupByIdentifier so
cs.addAPICalls((len(asins)+19)/20) is executed for every lookup attempt
(success, empty result, or error), e.g., call cs.addAPICalls immediately after
the LookupByIdentifier return before branching on err and enriched, keeping the
existing enrichment merges (products update), logging, and error handling
intact.

---

Duplicate comments:
In `@apps/web/src/components/discovery-product-table.tsx`:
- Around line 67-70: The csvCell function's formula-guard currently checks only
the first character and misses payloads with leading whitespace/control chars
(e.g. "\t=..."); update csvCell so it detects formula initiators after any
leading whitespace/control characters and prefixes a single quote immediately
before the formula character while preserving the original leading whitespace,
then continue to escape internal quotes and wrap in double quotes as before
(operate on variables raw and guarded inside csvCell and keep the final
replace(/"/g, '""') behavior).

In `@internal/service/assessment_service.go`:
- Around line 354-355: Several SP-API calls (e.g., spapi.SearchProducts) are
using a hardcoded "US" marketplace; replace that literal with the
tenant-specific marketplace value and thread it through the scan flow. Locate
calls to spapi.SearchProducts and the other SP-API calls flagged (the
surrounding code that also calls cs.addAPICalls) and change the marketplace
argument from "US" to the tenant marketplace variable (for example
tenant.Marketplace or a marketplace string passed into the scan function),
ensure the marketplace is passed down the call chain where the scan is initiated
so functions that call spapi.SearchProducts,
discovery/enrichment/eligibility/detail checks use that marketplace value
instead of "US".
- Around line 300-325: The jump loop never finds a candidate when scanning in
descending ExpectedOpen because it requires
DiscoveryCategories[jumpIdx].ExpectedOpen > cat.ExpectedOpen; change the
condition to allow equal-rate jumps (e.g. >=) and ensure you don't jump to the
same category by also checking jumpIdx != current category index (or another
available identity for cat) so the repeated-failure jump path can be taken;
update the condition in the loop that picks jumpIdx (and keep existing
scanned[jumpIdx] and cs.shouldStop() checks) so scanOneCategory is actually
invoked for equal-rate categories and cs.consecutiveEmptyCats logic remains
correct.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: e1d7a8e0-ed96-4cd8-a8eb-2c220df9137b

📥 Commits

Reviewing files that changed from the base of the PR and between c1df8a4 and c4fd094.

📒 Files selected for processing (4)

• apps/web/src/components/discovery-graph.tsx
• apps/web/src/components/discovery-product-table.tsx
• internal/service/assessment_hub.go
• internal/service/assessment_service.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/service/assessment_hub.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Stop scanning promptly when context is canceled.

The loops gate only on circuit breakers. If ctx is canceled/deadline-exceeded, the scan continues iterating and logging instead of terminating early.

Suggested fix

 	for _, idx := range scanOrder {
+		if err := ctx.Err(); err != nil {
+			cs.fireBreaker("context_canceled_or_deadline_exceeded")
+			break
+		}
 		if cs.shouldStop() {
@@
 	for _, p := range products {
+		if err := ctx.Err(); err != nil {
+			break
+		}
 		if cs.shouldStop() {
 			break
 		}

As per coding guidelines, “Context propagation is required everywhere in Go code”.

Also applies to: 402-405

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/assessment_service.go` around lines 255 - 267, The scan loop
in the function iterating over scanOrder only checks circuit-breaker methods
(cs.shouldStop(), cs.budgetExhausted(), cs.timeExceeded(), cs.earlySuccess())
and doesn't honor context cancellation; update the loop to check the request
context (e.g., cs.ctx or a passed-in ctx) by testing ctx.Err() or selecting on
ctx.Done() at the top of the loop and immediately break/return when canceled so
the scan stops promptly; ensure the same context check is added alongside the
existing checks (near the scanOrder loop and also at the other referenced spot
around lines 402-405) and use existing methods like cs.fireBreaker only after
detecting cancellation if you still need to log a breaker event.

[coderabbitai]

Actionable comments posted: 4

♻️ Duplicate comments (6)

scripts/start.sh (2)

99-110: ⚠️ Potential issue | 🟠 Major

Fail fast if frontend never becomes ready.

Lines 99-105 can time out and still continue to the success banner on Lines 107-110. Add an explicit readiness flag + exit path.

Suggested patch

-# Wait for frontend
-for i in $(seq 1 30); do
+# Wait for frontend
+WEB_READY=false
+for i in $(seq 1 30); do
   if curl -s http://localhost:3001 > /dev/null 2>&1; then
     echo -e "     ${GREEN}Frontend ready (PID: $WEB_PID).${NC}"
+    WEB_READY=true
     break
   fi
   sleep 1
 done
+
+if [ "$WEB_READY" = false ]; then
+  echo -e "     ${RED}Frontend failed to start. Check /tmp/fba-web.log${NC}"
+  exit 1
+fi

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/start.sh` around lines 99 - 110, The readiness loop that checks the
frontend (the for loop using curl) can time out yet the script still prints the
success banner; fix this by introducing an explicit readiness flag (e.g.,
ready=0) before the loop, set ready=1 inside the successful branch where curl
returns true (the same place that echoes "Frontend ready (PID: $WEB_PID)"),
break, and after the loop test the flag: if ready is still 0 print an error
message to stderr and exit with a non-zero code; only print the "All services
running!" banner when ready=1.

---

70-70: ⚠️ Potential issue | 🟠 Major

Use INNGEST_SERVE_ORIGIN instead of INNGEST_SERVE_HOST.

Line 70 still exports INNGEST_SERVE_HOST; this can break callback registration if runtime expects INNGEST_SERVE_ORIGIN.

Suggested patch

-export INNGEST_SERVE_HOST="http://host.docker.internal:8081"
+export INNGEST_SERVE_ORIGIN="http://host.docker.internal:8081"

For Inngest local development callback registration, which env var is correct: INNGEST_SERVE_ORIGIN or INNGEST_SERVE_HOST?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@scripts/start.sh` at line 70, Replace the exported environment variable
INNGEST_SERVE_HOST with INNGEST_SERVE_ORIGIN in scripts/start.sh: change the
export statement that currently sets INNGEST_SERVE_HOST to export
INNGEST_SERVE_ORIGIN (keeping the same origin value with scheme and port, e.g.
"http://host.docker.internal:8081") and update any subsequent references in this
script or related startup logic that read INNGEST_SERVE_HOST to instead read
INNGEST_SERVE_ORIGIN so callback registration uses the correct env var.

internal/service/assessment_service.go (3)

384-400: ⚠️ Potential issue | 🟠 Major

Count lookup attempts even when enrichment fails or returns empty.

cs.addAPICalls(...) only runs on successful non-empty LookupByIdentifier responses, but failed and empty lookups still consume SP-API budget. That undercounts usage and lets cbAPIBudget scan longer than intended.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/assessment_service.go` around lines 384 - 400, The SP-API
lookup batch count is only recorded when enriched is non-empty; move the
cs.addAPICalls((len(asins)+19)/20) call so it always runs immediately after
calling spapi.LookupByIdentifier (regardless of err or enriched length) to
account for consumed budget; keep the existing enrichment logic that uses
enriched, enrichMap, and products as-is.

---

260-272: ⚠️ Potential issue | 🟠 Major

Stop scanning when ctx is canceled.

Both loops only check circuit breakers. After cancellation or deadline expiry, the assessment can keep iterating categories/products and emitting events until another breaker trips. Exit on ctx.Err() before starting the next category and before checking the next product.

As per coding guidelines, “Context propagation is required everywhere in Go code”.

Also applies to: 408-410

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/assessment_service.go` around lines 260 - 272, The loop that
iterates scanOrder in assess (the block using cs.shouldStop(),
cs.fireBreaker(...) and computing time/api/early-success) must also check the
request context for cancellation; before starting the next category iteration
check if ctx.Err() != nil and break/return to stop processing, and do the same
check before iterating each product in the inner product loop (the loop that
also uses cs.shouldStop() around products) so cancellation/deadline expiry stops
work immediately—add a ctx.Err() check and short-circuit identical to the
circuit-breaker checks where cs.shouldStop() is evaluated (also apply the same
change to the other loop that currently lacks the ctx check at the analogous
location referenced in the review).

---

301-329: ⚠️ Potential issue | 🟠 Major

The repeated-failure jump can't select a better category with this ordering.

scanOrder is already highest ExpectedOpen to lowest, so the DiscoveryCategories[jumpIdx].ExpectedOpen > cat.ExpectedOpen guard never matches any remaining unscanned category. When this breaker fires, the jump path is effectively dead code. Jump to the first remaining unscanned category instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/assessment_service.go` around lines 301 - 329, The jump
logic inside the repeated-failure breaker (triggered when
cs.consecutiveEmptyCats >= cbEmptyCategoryThreshold) is unreachable because
scanOrder is already sorted high-to-low and the guard
DiscoveryCategories[jumpIdx].ExpectedOpen > cat.ExpectedOpen never matches;
change the loop over scanOrder to pick the first remaining unscanned category
(ignore ExpectedOpen comparison) by iterating scanOrder, checking if
!scanned[jumpIdx], then call cs.shouldStop(), mark scanned[jumpIdx]=true, set
jumpCat := DiscoveryCategories[jumpIdx], call s.scanOneCategory(ctx, tenantID,
spapi, jumpCat, cs, marketplace), append results to allResults and stats to
catStats, reset cs.consecutiveEmptyCats if jumpResult.stat.eligible > 0, set
jumped=true and break; keep the surrounding fireBreaker, logging, and break
semantics intact.

internal/service/seller_account_service.go (1)

79-88: ⚠️ Potential issue | 🟠 Major

Don't ignore repo.Get failures in the upsert path.

If s.repo.Get(ctx, tenantID) fails for anything other than “not found”, this falls through into Create, masking the real error. The read-then-write sequence is also still non-atomic, so concurrent reconnects can race into conflicting create/update paths. Prefer a repo-level upsert/transaction and only treat an explicit not-found as the create case.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/seller_account_service.go` around lines 79 - 88, The upsert
currently ignores errors from s.repo.Get and can wrongly fall through to Create
and race; change the logic in the Upsert path to check the error returned by
s.repo.Get(ctx, tenantID): if err != nil and is not a clear NotFound sentinel,
return fmt.Errorf("get seller account: %w", err); only treat an explicit
NotFound as the create branch. Better yet implement or call a repo-level Upsert
(e.g., s.repo.Upsert(ctx, account)) or perform the read-modify-write inside a
repo transaction (use repository transaction or locking) so concurrent
reconnects cannot race between Create and Update; update callers to use
s.repo.Upsert or the transactional methods instead of relying on in-service
read-then-write.

🧹 Nitpick comments (1)

apps/web/src/lib/types.ts (1)

424-453: Tighten SSE typing by making SSEEvent a discriminated union to eliminate unsafe type assertions.

The current SSEEvent.data: Record<string, unknown> forces downstream code to cast properties. In use-assessment-sse.ts, multiple as string casts are used (lines 77, 78, 185, 236) when accessing fields like asin, category, and subcategory. Type guards already check evt.type, but TypeScript cannot narrow the data shape.

A discriminated union ensures type safety:

♻️ Proposed type refinement

+export type SSEEvent =
+  | { type: "product_found"; timestamp: string; data: SSEProductFound }
+  | { type: "category_start"; timestamp: string; data: SSECategoryStart }
+  | { type: "category_complete"; timestamp: string; data: SSECategoryComplete };
-
-export interface SSEEvent {
-  type: string;
-  timestamp: string;
-  data: Record<string, unknown>;
-}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/lib/types.ts` around lines 424 - 453, SSEEvent currently uses
data: Record<string, unknown>, forcing unsafe casts in use-assessment-sse.ts;
replace SSEEvent with a discriminated union of specific event interfaces (e.g.,
SSEProductFoundEvent { type: "product_found"; data: SSEProductFound },
SSECategoryStartEvent { type: "category_start"; data: SSECategoryStart },
SSECategoryCompleteEvent { type: "category_complete"; data: SSECategoryComplete
} and a fallback GenericSSEEvent for unknown types) so that switches/guards on
evt.type in use-assessment-sse.ts can auto-narrow evt.data and remove all `as
string`/casts (refer to SSEEvent, SSEProductFound, SSECategoryStart,
SSECategoryComplete and the handlers in use-assessment-sse.ts).

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@internal/adapter/inngest/client.go`:
- Around line 927-930: The code currently calls
assessmentSvc.FailAssessment(ctx, tenantID) and then ignores its error,
returning a workflow status map which can leave the persisted profile stuck;
change both occurrences (the tenantSPAPI nil branch and the similar block around
lines 943-945) to capture the error returned from assessmentSvc.FailAssessment,
and if non-nil log the error (include "err", err) and return that error to the
caller (instead of swallowing it and returning the status map), otherwise
proceed to return the failure status map; reference
assessmentSvc.FailAssessment, tenantID, tenantSPAPI and the existing return map
to locate and update the logic.
- Around line 921-935: Replace the shared singleton assignment of tenantSPAPI
(currently set to productSearcher) with code that builds a per-tenant SP-API
client from the tenant’s stored seller credentials and use that client in
RunDiscoveryAssessment; specifically, call the per-tenant client builder (e.g.,
credentialSvc.GetSPAPIClient(ctx, tenantID) or the equivalent function that
constructs an SP-API client from the tenant’s seller account), handle and log
any error or nil client the same way the current nil-check does (slog.Error +
assessmentSvc.FailAssessment + return failure map), and then pass the resulting
tenant-specific client into assessmentSvc.RunDiscoveryAssessment instead of the
global productSearcher.

In `@internal/service/assessment_service_test.go`:
- Around line 551-565: The test currently lets missing breakers pass silently;
update the assertions in internal/service/assessment_service_test.go to
explicitly assert the expected breaker key and behavior instead of falling back
to t.Log/no-op: check outcome.CircuitBreakers contains the exact per-category
breaker string (e.g. "per_category_skip") and assert its entry length equals the
expected number of fired checks (e.g. 5), and do the same for the other
occurrence referenced (lines ~669-680) to ensure the specific breaker is present
and has the expected count rather than permitting a silent pass.

In `@internal/service/seller_account_service.go`:
- Around line 97-102: GetAccount currently forwards s.repo.Get such that a
missing tenant row is returned as an error; change GetAccount to detect the repo
"not found" error (the sentinel or sql.ErrNoRows used by the Postgres repo) and
return (nil, nil) instead of propagating that error so disconnected sellers are
represented as nil without error. In practice, update GetAccount to call
s.repo.Get(ctx, tenantID), check if err indicates "not found" (the repo's
specific sentinel like ErrNotFound or sql.ErrNoRows) and return nil, nil; only
return other errors as-is. Ensure you reference SellerAccountService.GetAccount
and s.repo.Get when applying the change.

---

Duplicate comments:
In `@internal/service/assessment_service.go`:
- Around line 384-400: The SP-API lookup batch count is only recorded when
enriched is non-empty; move the cs.addAPICalls((len(asins)+19)/20) call so it
always runs immediately after calling spapi.LookupByIdentifier (regardless of
err or enriched length) to account for consumed budget; keep the existing
enrichment logic that uses enriched, enrichMap, and products as-is.
- Around line 260-272: The loop that iterates scanOrder in assess (the block
using cs.shouldStop(), cs.fireBreaker(...) and computing time/api/early-success)
must also check the request context for cancellation; before starting the next
category iteration check if ctx.Err() != nil and break/return to stop
processing, and do the same check before iterating each product in the inner
product loop (the loop that also uses cs.shouldStop() around products) so
cancellation/deadline expiry stops work immediately—add a ctx.Err() check and
short-circuit identical to the circuit-breaker checks where cs.shouldStop() is
evaluated (also apply the same change to the other loop that currently lacks the
ctx check at the analogous location referenced in the review).
- Around line 301-329: The jump logic inside the repeated-failure breaker
(triggered when cs.consecutiveEmptyCats >= cbEmptyCategoryThreshold) is
unreachable because scanOrder is already sorted high-to-low and the guard
DiscoveryCategories[jumpIdx].ExpectedOpen > cat.ExpectedOpen never matches;
change the loop over scanOrder to pick the first remaining unscanned category
(ignore ExpectedOpen comparison) by iterating scanOrder, checking if
!scanned[jumpIdx], then call cs.shouldStop(), mark scanned[jumpIdx]=true, set
jumpCat := DiscoveryCategories[jumpIdx], call s.scanOneCategory(ctx, tenantID,
spapi, jumpCat, cs, marketplace), append results to allResults and stats to
catStats, reset cs.consecutiveEmptyCats if jumpResult.stat.eligible > 0, set
jumped=true and break; keep the surrounding fireBreaker, logging, and break
semantics intact.

In `@internal/service/seller_account_service.go`:
- Around line 79-88: The upsert currently ignores errors from s.repo.Get and can
wrongly fall through to Create and race; change the logic in the Upsert path to
check the error returned by s.repo.Get(ctx, tenantID): if err != nil and is not
a clear NotFound sentinel, return fmt.Errorf("get seller account: %w", err);
only treat an explicit NotFound as the create branch. Better yet implement or
call a repo-level Upsert (e.g., s.repo.Upsert(ctx, account)) or perform the
read-modify-write inside a repo transaction (use repository transaction or
locking) so concurrent reconnects cannot race between Create and Update; update
callers to use s.repo.Upsert or the transactional methods instead of relying on
in-service read-then-write.

In `@scripts/start.sh`:
- Around line 99-110: The readiness loop that checks the frontend (the for loop
using curl) can time out yet the script still prints the success banner; fix
this by introducing an explicit readiness flag (e.g., ready=0) before the loop,
set ready=1 inside the successful branch where curl returns true (the same place
that echoes "Frontend ready (PID: $WEB_PID)"), break, and after the loop test
the flag: if ready is still 0 print an error message to stderr and exit with a
non-zero code; only print the "All services running!" banner when ready=1.
- Line 70: Replace the exported environment variable INNGEST_SERVE_HOST with
INNGEST_SERVE_ORIGIN in scripts/start.sh: change the export statement that
currently sets INNGEST_SERVE_HOST to export INNGEST_SERVE_ORIGIN (keeping the
same origin value with scheme and port, e.g. "http://host.docker.internal:8081")
and update any subsequent references in this script or related startup logic
that read INNGEST_SERVE_HOST to instead read INNGEST_SERVE_ORIGIN so callback
registration uses the correct env var.

---

Nitpick comments:
In `@apps/web/src/lib/types.ts`:
- Around line 424-453: SSEEvent currently uses data: Record<string, unknown>,
forcing unsafe casts in use-assessment-sse.ts; replace SSEEvent with a
discriminated union of specific event interfaces (e.g., SSEProductFoundEvent {
type: "product_found"; data: SSEProductFound }, SSECategoryStartEvent { type:
"category_start"; data: SSECategoryStart }, SSECategoryCompleteEvent { type:
"category_complete"; data: SSECategoryComplete } and a fallback GenericSSEEvent
for unknown types) so that switches/guards on evt.type in use-assessment-sse.ts
can auto-narrow evt.data and remove all `as string`/casts (refer to SSEEvent,
SSEProductFound, SSECategoryStart, SSECategoryComplete and the handlers in
use-assessment-sse.ts).

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 8caeeafb-109a-4e87-8068-7739b3652509

📥 Commits

Reviewing files that changed from the base of the PR and between c4fd094 and 2adf9a6.

📒 Files selected for processing (10)

• apps/web/src/app/(app)/onboarding/page.tsx
• apps/web/src/lib/api-client.ts
• apps/web/src/lib/types.ts
• internal/adapter/inngest/client.go
• internal/adapter/postgres/migrations/020_rls_force_seller_accounts.sql
• internal/domain/crypto.go
• internal/service/assessment_service.go
• internal/service/assessment_service_test.go
• internal/service/seller_account_service.go
• scripts/start.sh

✅ Files skipped from review due to trivial changes (1)

• internal/adapter/postgres/migrations/020_rls_force_seller_accounts.sql

🚧 Files skipped from review as they are similar to previous changes (3)

• internal/domain/crypto.go
• apps/web/src/lib/api-client.ts
• apps/web/src/app/(app)/onboarding/page.tsx

[coderabbitai]

⚠️ Potential issue | 🟠 Major

tenantSPAPI is still the shared singleton.

This assigns the injected productSearcher directly even though RunDiscoveryAssessment explicitly expects a per-tenant SP-API client. As written, every seller assessment runs against the shared/global client instead of the tenant’s connected account and marketplace, which breaks the per-seller credential model introduced in this PR. Build the client from the tenant’s stored seller account before calling RunDiscoveryAssessment.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/adapter/inngest/client.go` around lines 921 - 935, Replace the
shared singleton assignment of tenantSPAPI (currently set to productSearcher)
with code that builds a per-tenant SP-API client from the tenant’s stored seller
credentials and use that client in RunDiscoveryAssessment; specifically, call
the per-tenant client builder (e.g., credentialSvc.GetSPAPIClient(ctx, tenantID)
or the equivalent function that constructs an SP-API client from the tenant’s
seller account), handle and log any error or nil client the same way the current
nil-check does (slog.Error + assessmentSvc.FailAssessment + return failure map),
and then pass the resulting tenant-specific client into
assessmentSvc.RunDiscoveryAssessment instead of the global productSearcher.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't drop FailAssessment errors.

If assessmentSvc.FailAssessment fails here, the workflow returns a failed status but the persisted profile can stay stuck in running. Propagate that error, or at least surface it and fail the function, so the onboarding state can't silently diverge from the workflow result.

Also applies to: 943-945

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/adapter/inngest/client.go` around lines 927 - 930, The code
currently calls assessmentSvc.FailAssessment(ctx, tenantID) and then ignores its
error, returning a workflow status map which can leave the persisted profile
stuck; change both occurrences (the tenantSPAPI nil branch and the similar block
around lines 943-945) to capture the error returned from
assessmentSvc.FailAssessment, and if non-nil log the error (include "err", err)
and return that error to the caller (instead of swallowing it and returning the
status map), otherwise proceed to return the failure status map; reference
assessmentSvc.FailAssessment, tenantID, tenantSPAPI and the existing return map
to locate and update the logic.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

These breaker tests can pass without proving the breaker fired.

Both cases fall back to t.Log/no-op when the expected breaker is missing, so a regression in the breaker logic still yields green tests. Make them assert on the specific breaker string they expect, otherwise this coverage won't protect the new scan-control behavior.

Also applies to: 669-680

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/assessment_service_test.go` around lines 551 - 565, The test
currently lets missing breakers pass silently; update the assertions in
internal/service/assessment_service_test.go to explicitly assert the expected
breaker key and behavior instead of falling back to t.Log/no-op: check
outcome.CircuitBreakers contains the exact per-category breaker string (e.g.
"per_category_skip") and assert its entry length equals the expected number of
fired checks (e.g. 5), and do the same for the other occurrence referenced
(lines ~669-680) to ensure the specific breaker is present and has the expected
count rather than permitting a silent pass.

[coderabbitai]

Actionable comments posted: 6

♻️ Duplicate comments (4)

internal/adapter/inngest/client.go (2)

927-932: ⚠️ Potential issue | 🟠 Major

Failure paths still swallow FailAssessment write errors.

Both branches log the secondary failure but then return a normal "failed" payload. If that profile update fails, the workflow reports failure while the persisted assessment can remain running.

Suggested fix

 				if tenantSPAPI == nil {
 					slog.Error("inngest[run-assessment]: no SP-API client available", "tenant_id", data.TenantID)
 					if err := assessmentSvc.FailAssessment(ctx, tenantID); err != nil {
 						slog.Error("inngest[run-assessment]: FailAssessment also failed", "error", err)
+						return nil, fmt.Errorf("mark assessment failed: %w", err)
 					}
 					return map[string]string{"status": "failed", "error": "no SP-API client"}, nil
 				}
@@
 				if err != nil {
 					slog.Error("inngest[run-assessment]: discovery failed", "tenant_id", data.TenantID, "error", err)
 					if fErr := assessmentSvc.FailAssessment(ctx, tenantID); fErr != nil {
 						slog.Error("inngest[run-assessment]: FailAssessment also failed", "error", fErr)
+						return nil, fmt.Errorf("discovery failed: %w; mark-failed also failed: %w", err, fErr)
 					}
 					return map[string]string{"status": "failed", "error": err.Error()}, nil
 				}

Also applies to: 945-949

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/adapter/inngest/client.go` around lines 927 - 932, The code handles
missing SP-API clients by calling assessmentSvc.FailAssessment(ctx, tenantID)
but swallows errors from that call and returns a normal payload; change both
failure branches (the block checking tenantSPAPI == nil and the analogous later
branch) to propagate the FailAssessment error to the caller: if
assessmentSvc.FailAssessment returns an error, log it and return nil plus that
error (not a normal "failed" payload), otherwise return the existing failure
payload; ensure you update the branches referencing tenantSPAPI and the
invocation of assessmentSvc.FailAssessment so the function returns an error when
the DB/profile update fails.

---

921-937: ⚠️ Potential issue | 🟠 Major

The assessment still uses the shared SP-API client and US fallback marketplace.

tenantSPAPI := productSearcher plus the empty marketplace argument means this path is still pinned to the env-backed singleton and "US" default, so tenant-specific credentials/catalogs are not actually honored here.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/adapter/inngest/client.go` around lines 921 - 937, Summary:
tenantSPAPI is still assigned the shared productSearcher and
RunDiscoveryAssessment is called with an empty marketplace, so tenant
credentials and marketplace are not honored. Fix: replace tenantSPAPI :=
productSearcher with a call to credentialSvc.GetSPAPIClient(ctx, tenantID) and
handle nil/error exactly like the existing nil-path (log and FailAssessment);
then pass an explicit marketplace value to assessmentSvc.RunDiscoveryAssessment
instead of "" — use data.Marketplace if present or fetch tenant-specific
marketplace (e.g., tenantSvc.GetMarketplace(ctx, tenantID)) and fall back to
"US" only as a last resort. Ensure you reference tenantSPAPI,
credentialSvc.GetSPAPIClient, assessmentSvc.RunDiscoveryAssessment,
data.Marketplace (or tenantSvc.GetMarketplace) in the change.

internal/service/assessment_service.go (2)

414-421: ⚠️ Potential issue | 🟠 Major

Per-category scanning still keeps calling SP-API after cancellation.

Once a category scan starts, this loop only checks breaker state. If ctx is canceled mid-category, it can still issue more eligibility checks and then the trailing GetProductDetails batch.

Suggested fix

 	for _, p := range products {
+		if err := ctx.Err(); err != nil {
+			cs.fireBreaker("context_canceled")
+			break
+		}
 		if cs.shouldStop() {
 			break
 		}
@@
-	if len(eligibleASINs) > 0 {
+	if ctx.Err() == nil && len(eligibleASINs) > 0 {
 		enriched, err := spapi.GetProductDetails(ctx, eligibleASINs, marketplace)

As per coding guidelines, “Context propagation is required everywhere in Go code”.

Also applies to: 508-510

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/assessment_service.go` around lines 414 - 421, The loop that
calls spapi.CheckListingEligibility and later spapi.GetProductDetails must stop
immediately if the provided ctx is canceled; update the per-category scan in
AssessProduct(s) so you check ctx.Err() (or select on ctx.Done()) just before
each external SP-API call (e.g., before spapi.CheckListingEligibility and before
the batched spapi.GetProductDetails) and return/break if canceled, rather than
relying solely on cs.shouldStop(); apply the same ctx-check pattern to the
similar block around the GetProductDetails batch (the code referenced at lines
~508-510) so no further API calls are made after context cancellation.

---

306-335: ⚠️ Potential issue | 🟠 Major

The empty-category jump breaker is still unreachable.

scanOrder already iterates categories from highest to lowest ExpectedOpen, so an unscanned entry can never satisfy ExpectedOpen > cat.ExpectedOpen. After three empty categories this path just logs and never actually reprioritizes.

Suggested fix

-			for _, jumpIdx := range scanOrder {
-				if !scanned[jumpIdx] && DiscoveryCategories[jumpIdx].ExpectedOpen > cat.ExpectedOpen {
+			for _, jumpIdx := range scanOrder {
+				if !scanned[jumpIdx] {
 					// Reorder: put this one next
 					// We just continue the loop — the check at the top will skip scanned ones
 					// Actually we need to scan it now

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/assessment_service.go` around lines 306 - 335, The jump
logic never finds a candidate because scanOrder already goes highest->lowest and
the condition DiscoveryCategories[jumpIdx].ExpectedOpen > cat.ExpectedOpen is
always false for any unscanned entry; change the loop to select the unscanned
category with the maximum ExpectedOpen (no > cat.ExpectedOpen check) and then
scan that one. Concretely, in the block referencing scanOrder, scanned,
DiscoveryCategories and s.scanOneCategory, iterate scanOrder to find the
unscanned index with the largest DiscoveryCategories[j].ExpectedOpen (store
bestIdx), then if found mark scanned[bestIdx]=true, call s.scanOneCategory(ctx,
tenantID, spapi, DiscoveryCategories[bestIdx], cs, marketplace), append
jumpResult.results and jumpResult.stat, reset cs.consecutiveEmptyCats if
jumpResult.stat.eligible>0, set jumped=true; otherwise keep the existing log
path.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@apps/web/src/components/discovery-graph.tsx`:
- Around line 235-251: Update the legend JSX in discovery-graph.tsx to match the
graph's three-state coloring: add an explicit amber/ungatable state entry (e.g.,
a span with an amber dot like bg-amber-500) labeled "Ungatable / Can Apply"
(with its percent range or description) to reflect brand/product nodes, change
the existing yellow/red/green labels to match their exact meanings if needed,
and modify the gray "Subcategory" entry text to indicate subcategories are
dynamically color-coded (e.g., "Subcategory (dynamic color by eligible ratio)")
or remove the static gray dot so the legend no longer implies subcategories are
a fixed gray color; update the corresponding span elements in the same JSX block
so the UI legend and the graph coloring stay consistent.

In `@docs/strategy/go-to-market-strategy.md`:
- Line 42: In the table row containing "SellerAmp / BuyBotPro | Calculator |
Single-ASIN profitability scoring via chrome extension | No batch processing, no
pipeline, no eligibility", fix the product-name casing by changing "chrome
extension" to "Chrome extension" so the entry reads "...Single-ASIN
profitability scoring via Chrome extension...".

In `@docs/strategy/risks-and-improvements.md`:
- Line 203: Update the list item text "Error type discrimination at handler
layer" to use a hyphenated compound modifier for readability, e.g., change it to
"Error-type discrimination at handler layer"; locate and edit that exact string
in the document (the list item heading) so only the wording is adjusted and
formatting/spacing remains unchanged.
- Line 22: Update the sentence that claims ValidateAgentOutput() is “never
called in production paths” to be accurate: change it to state that
ValidateAgentOutput() is called in the production reviewer flow (see
internal/service/reviewer.go, function handling reviewer flow around lines
61-73) but is not invoked in pipeline orchestration paths such as RunPipeline
and EvaluateCandidate; specifically, edit the text to narrow the scope to
“pipeline orchestration paths (RunPipeline/EvaluateCandidate)” rather than
making an absolute claim about production usage.

In `@internal/service/assessment_service.go`:
- Around line 168-172: Start threading the request context through the hub API:
change StartStream(tenantID), EndStream(tenantID) and emitEvent(...) usages to
accept ctx (e.g., StartStream(ctx, tenantID), EndStream(ctx, tenantID),
emitEvent(ctx, ...)) and update their implementations to honor ctx cancellations
by returning early if ctx.Err() != nil before doing work or publishing;
alternatively, if you can't change signatures everywhere, add a ctx check before
calling the hub methods (short-circuit when ctx.Err() != nil) so canceled
assessments stop SSE work—apply the same pattern to
StartStream/EndStream/emitEvent and the other occurrences around lines 944-952.
- Around line 389-406: The code currently calls cs.addAPICalls for the
LookupByIdentifier batch twice (once immediately after spapi.LookupByIdentifier
and again inside the success branch), which double-counts API budget; remove the
redundant cs.addAPICalls((len(asins) + 19) / 20) call inside the if err == nil
&& len(enriched) > 0 block so the batch cost is only recorded once after
spapi.LookupByIdentifier, leaving the enrichment logic (enrichMap creation and
products update) unchanged.

---

Duplicate comments:
In `@internal/adapter/inngest/client.go`:
- Around line 927-932: The code handles missing SP-API clients by calling
assessmentSvc.FailAssessment(ctx, tenantID) but swallows errors from that call
and returns a normal payload; change both failure branches (the block checking
tenantSPAPI == nil and the analogous later branch) to propagate the
FailAssessment error to the caller: if assessmentSvc.FailAssessment returns an
error, log it and return nil plus that error (not a normal "failed" payload),
otherwise return the existing failure payload; ensure you update the branches
referencing tenantSPAPI and the invocation of assessmentSvc.FailAssessment so
the function returns an error when the DB/profile update fails.
- Around line 921-937: Summary: tenantSPAPI is still assigned the shared
productSearcher and RunDiscoveryAssessment is called with an empty marketplace,
so tenant credentials and marketplace are not honored. Fix: replace tenantSPAPI
:= productSearcher with a call to credentialSvc.GetSPAPIClient(ctx, tenantID)
and handle nil/error exactly like the existing nil-path (log and
FailAssessment); then pass an explicit marketplace value to
assessmentSvc.RunDiscoveryAssessment instead of "" — use data.Marketplace if
present or fetch tenant-specific marketplace (e.g.,
tenantSvc.GetMarketplace(ctx, tenantID)) and fall back to "US" only as a last
resort. Ensure you reference tenantSPAPI, credentialSvc.GetSPAPIClient,
assessmentSvc.RunDiscoveryAssessment, data.Marketplace (or
tenantSvc.GetMarketplace) in the change.

In `@internal/service/assessment_service.go`:
- Around line 414-421: The loop that calls spapi.CheckListingEligibility and
later spapi.GetProductDetails must stop immediately if the provided ctx is
canceled; update the per-category scan in AssessProduct(s) so you check
ctx.Err() (or select on ctx.Done()) just before each external SP-API call (e.g.,
before spapi.CheckListingEligibility and before the batched
spapi.GetProductDetails) and return/break if canceled, rather than relying
solely on cs.shouldStop(); apply the same ctx-check pattern to the similar block
around the GetProductDetails batch (the code referenced at lines ~508-510) so no
further API calls are made after context cancellation.
- Around line 306-335: The jump logic never finds a candidate because scanOrder
already goes highest->lowest and the condition
DiscoveryCategories[jumpIdx].ExpectedOpen > cat.ExpectedOpen is always false for
any unscanned entry; change the loop to select the unscanned category with the
maximum ExpectedOpen (no > cat.ExpectedOpen check) and then scan that one.
Concretely, in the block referencing scanOrder, scanned, DiscoveryCategories and
s.scanOneCategory, iterate scanOrder to find the unscanned index with the
largest DiscoveryCategories[j].ExpectedOpen (store bestIdx), then if found mark
scanned[bestIdx]=true, call s.scanOneCategory(ctx, tenantID, spapi,
DiscoveryCategories[bestIdx], cs, marketplace), append jumpResult.results and
jumpResult.stat, reset cs.consecutiveEmptyCats if jumpResult.stat.eligible>0,
set jumped=true; otherwise keep the existing log path.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 44b9a39c-c957-478a-9041-f2403e866cb0

📥 Commits

Reviewing files that changed from the base of the PR and between 2adf9a6 and 8759726.

📒 Files selected for processing (6)

• apps/web/src/components/discovery-graph.tsx
• docs/strategy/go-to-market-strategy.md
• docs/strategy/risks-and-improvements.md
• internal/adapter/inngest/client.go
• internal/service/assessment_service.go
• internal/service/seller_account_service.go

🚧 Files skipped from review as they are similar to previous changes (1)

• internal/service/seller_account_service.go

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Legend is out of sync with the graph’s 3-state coloring.

Line 241–251 does not explain the explicit ungatable/can-apply amber state used by brand/product nodes (and the gray “Subcategory” dot is misleading since subcategories are dynamically color-coded). This can confuse users interpreting results.

Suggested patch

       <div className="mt-2 flex flex-wrap gap-3 text-xs text-muted-foreground">
         <span className="flex items-center gap-1">
           <span className="inline-block h-2.5 w-2.5 rounded-full bg-green-500" /> Eligible / Open
           &gt;50%
         </span>
+        <span className="flex items-center gap-1">
+          <span className="inline-block h-2.5 w-2.5 rounded-full bg-amber-500" /> Can Apply
+          (Ungatable)
+        </span>
         <span className="flex items-center gap-1">
           <span className="inline-block h-2.5 w-2.5 rounded-full bg-yellow-500" /> Partially Open
           20-50%
         </span>
         <span className="flex items-center gap-1">
           <span className="inline-block h-2.5 w-2.5 rounded-full bg-red-500" /> Restricted / Open
           &lt;20%
         </span>
-        <span className="flex items-center gap-1">
-          <span className="inline-block h-2.5 w-2.5 rounded-full bg-gray-400" /> Subcategory
-          (colored by eligible ratio)
-        </span>

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@apps/web/src/components/discovery-graph.tsx` around lines 235 - 251, Update
the legend JSX in discovery-graph.tsx to match the graph's three-state coloring:
add an explicit amber/ungatable state entry (e.g., a span with an amber dot like
bg-amber-500) labeled "Ungatable / Can Apply" (with its percent range or
description) to reflect brand/product nodes, change the existing
yellow/red/green labels to match their exact meanings if needed, and modify the
gray "Subcategory" entry text to indicate subcategories are dynamically
color-coded (e.g., "Subcategory (dynamic color by eligible ratio)") or remove
the static gray dot so the legend no longer implies subcategories are a fixed
gray color; update the corresponding span elements in the same JSX block so the
UI legend and the graph coloring stay consistent.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Capitalize the product name “Chrome.”

Line 42 uses “chrome extension”; this should be “Chrome extension” for correct product-name casing.

Suggested fix

-| SellerAmp / BuyBotPro | Calculator | Single-ASIN profitability scoring via chrome extension | No batch processing, no pipeline, no eligibility |
+| SellerAmp / BuyBotPro | Calculator | Single-ASIN profitability scoring via Chrome extension | No batch processing, no pipeline, no eligibility |

🧰 Tools

🪛 LanguageTool

[uncategorized] ~42-~42: The proper noun “Chrome” (= software from Google) needs to be capitalized.
Context: ...| Single-ASIN profitability scoring via chrome extension | No batch processing, no pip...

(GOOGLE_PRODUCTS)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/strategy/go-to-market-strategy.md` at line 42, In the table row
containing "SellerAmp / BuyBotPro | Calculator | Single-ASIN profitability
scoring via chrome extension | No batch processing, no pipeline, no
eligibility", fix the product-name casing by changing "chrome extension" to
"Chrome extension" so the entry reads "...Single-ASIN profitability scoring via
Chrome extension...".

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Correct the absolute claim about ValidateAgentOutput() usage.

Line 22 says it is “never called in production paths,” but internal/service/reviewer.go (Line 61-73) shows it is called in production reviewer flow. Please narrow the statement to pipeline orchestration paths (RunPipeline/EvaluateCandidate) to keep this doc accurate.

Suggested wording update

- `ValidateAgentOutput()` exists in `internal/domain/validation.go` but is never called in production paths. An LLM returning `"passed": "yes"` instead of `"passed": true` silently eliminates every candidate in a run with no error, no log, no alert.
+ `ValidateAgentOutput()` exists in `internal/domain/validation.go` but is not called in the main orchestration paths (`RunPipeline` / `EvaluateCandidate`). An LLM returning `"passed": "yes"` instead of `"passed": true` can silently eliminate candidates in a run with no explicit validation failure signal.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	`ValidateAgentOutput()` exists in `internal/domain/validation.go` but is never called in production paths. An LLM returning `"passed": "yes"` instead of `"passed": true` silently eliminates every candidate in a run with no error, no log, no alert.
	`ValidateAgentOutput()` exists in `internal/domain/validation.go` but is not called in the main orchestration paths (`RunPipeline` / `EvaluateCandidate`). An LLM returning `"passed": "yes"` instead of `"passed": true` can silently eliminate candidates in a run with no explicit validation failure signal.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/strategy/risks-and-improvements.md` at line 22, Update the sentence that
claims ValidateAgentOutput() is “never called in production paths” to be
accurate: change it to state that ValidateAgentOutput() is called in the
production reviewer flow (see internal/service/reviewer.go, function handling
reviewer flow around lines 61-73) but is not invoked in pipeline orchestration
paths such as RunPipeline and EvaluateCandidate; specifically, edit the text to
narrow the scope to “pipeline orchestration paths
(RunPipeline/EvaluateCandidate)” rather than making an absolute claim about
production usage.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Minor wording polish: prefer hyphenated compound modifier.

Use a hyphenated form for readability in the list item text.

Suggested wording update

- 10. Error type discrimination at handler layer
+ 10. Error-type discrimination at handler layer

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	10. Error type discrimination at handler layer
	10. Error-type discrimination at handler layer

🧰 Tools

🪛 LanguageTool

[grammar] ~203-~203: Use a hyphen to join words.
Context: ...event payloads with versioning 10. Error type discrimination at handler layer --...

(QB_NEW_EN_HYPHEN)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/strategy/risks-and-improvements.md` at line 203, Update the list item
text "Error type discrimination at handler layer" to use a hyphenated compound
modifier for readability, e.g., change it to "Error-type discrimination at
handler layer"; locate and edit that exact string in the document (the list item
heading) so only the wording is adjusted and formatting/spacing remains
unchanged.

[coderabbitai]

🛠️ Refactor suggestion | 🟠 Major

Thread ctx through the hub boundary as well.

StartStream, EndStream, and emitEvent drop the request context entirely, so canceled assessments can keep doing SSE work even after the main flow stops. Please make the hub lifecycle/publish API context-aware, or at minimum short-circuit on ctx.Err() before publishing.

As per coding guidelines, “Context propagation is required everywhere in Go code”.

Also applies to: 944-952

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/assessment_service.go` around lines 168 - 172, Start
threading the request context through the hub API: change StartStream(tenantID),
EndStream(tenantID) and emitEvent(...) usages to accept ctx (e.g.,
StartStream(ctx, tenantID), EndStream(ctx, tenantID), emitEvent(ctx, ...)) and
update their implementations to honor ctx cancellations by returning early if
ctx.Err() != nil before doing work or publishing; alternatively, if you can't
change signatures everywhere, add a ctx check before calling the hub methods
(short-circuit when ctx.Err() != nil) so canceled assessments stop SSE
work—apply the same pattern to StartStream/EndStream/emitEvent and the other
occurrences around lines 944-952.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

LookupByIdentifier successes are counted twice against the API budget.

The batch cost is added once immediately after the call and again in the success branch. Successful enrichments now burn double budget and can trip cbAPIBudget early.

Suggested fix

 	enriched, err := spapi.LookupByIdentifier(ctx, asins, "ASIN", marketplace)
 	cs.addAPICalls((len(asins) + 19) / 20) // batch of 20
 	if err == nil && len(enriched) > 0 {
 		enrichMap := make(map[string]port.ProductSearchResult)
@@
-		cs.addAPICalls((len(asins) + 19) / 20) // batch calls (20 per request)
 		slog.Info("assessment: brand enrichment complete", "category", cat.Name, "enriched", len(enriched))
 	} else if err != nil {

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	enriched, err := spapi.LookupByIdentifier(ctx, asins, "ASIN", marketplace)
	cs.addAPICalls((len(asins) + 19) / 20) // batch of 20
	if err == nil && len(enriched) > 0 {
	enrichMap := make(map[string]port.ProductSearchResult)
	for _, e := range enriched {
	enrichMap[e.ASIN] = e
	}
	for i, p := range products {
	if e, ok := enrichMap[p.ASIN]; ok {
	if e.Brand != "" && products[i].Brand == "" {
	products[i].Brand = e.Brand
	}
	if e.AmazonPrice > 0 && products[i].AmazonPrice <= 0 {
	products[i].AmazonPrice = e.AmazonPrice
	}
	}
	}
	cs.addAPICalls((len(asins) + 19) / 20) // batch calls (20 per request)
	enriched, err := spapi.LookupByIdentifier(ctx, asins, "ASIN", marketplace)
	cs.addAPICalls((len(asins) + 19) / 20) // batch of 20
	if err == nil && len(enriched) > 0 {
	enrichMap := make(map[string]port.ProductSearchResult)
	for _, e := range enriched {
	enrichMap[e.ASIN] = e
	}
	for i, p := range products {
	if e, ok := enrichMap[p.ASIN]; ok {
	if e.Brand != "" && products[i].Brand == "" {
	products[i].Brand = e.Brand
	}
	if e.AmazonPrice > 0 && products[i].AmazonPrice <= 0 {
	products[i].AmazonPrice = e.AmazonPrice
	}
	}
	}
	slog.Info("assessment: brand enrichment complete", "category", cat.Name, "enriched", len(enriched))
	} else if err != nil {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/service/assessment_service.go` around lines 389 - 406, The code
currently calls cs.addAPICalls for the LookupByIdentifier batch twice (once
immediately after spapi.LookupByIdentifier and again inside the success branch),
which double-counts API budget; remove the redundant cs.addAPICalls((len(asins)
+ 19) / 20) call inside the if err == nil && len(enriched) > 0 block so the
batch cost is only recorded once after spapi.LookupByIdentifier, leaving the
enrichment logic (enrichMap creation and products update) unchanged.

[coderabbitai]

Actionable comments posted: 5

🧹 Nitpick comments (2)

internal/api/handler/assessment_handler.go (2)

29-33: Consider fully draining the request body before closing.

Simply calling r.Body.Close() without reading may leave unconsumed bytes on keep-alive connections. Use io.Copy(io.Discard, r.Body) to ensure the body is fully drained.

♻️ Suggested improvement

+	"io"
 	"log/slog"

 	// No longer collecting account age / listings / capital — inferred post-assessment
 	if r.Body != nil {
 		// Drain body for compatibility with old clients sending JSON
+		_, _ = io.Copy(io.Discard, r.Body)
 		r.Body.Close()
 	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/api/handler/assessment_handler.go` around lines 29 - 33, The handler
currently just calls r.Body.Close() which can leave unconsumed bytes on
keep-alive connections; before closing r.Body (the request body used in the
handler where the snippet lives) call io.Copy(io.Discard, r.Body) to fully drain
it and then close; handle or ignore the returned error appropriately (e.g., log
with the existing logger or discard) to avoid resource leaks and ensure proper
connection reuse.

---

444-463: Handle json.Marshal errors to avoid sending malformed SSE data.

While json.Marshal rarely fails for these types, ignoring errors could result in sending null data to clients. Consider logging marshal failures.

♻️ Suggested improvement

 	// Send catch-up history
 	if len(history) > 0 {
-		data, _ := json.Marshal(history)
+		data, err := json.Marshal(history)
+		if err != nil {
+			slog.Error("assessment: failed to marshal catchup", "error", err)
+			return
+		}
 		fmt.Fprintf(w, "event: catchup\ndata: %s\n\n", data)
 		flusher.Flush()
 	}

-			data, _ := json.Marshal(evt)
+			data, err := json.Marshal(evt)
+			if err != nil {
+				slog.Error("assessment: failed to marshal event", "type", evt.Type, "error", err)
+				continue
+			}
 			fmt.Fprintf(w, "event: %s\ndata: %s\n\n", evt.Type, data)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/api/handler/assessment_handler.go` around lines 444 - 463, The code
currently ignores errors from json.Marshal when serializing both the catch-up
history and each evt before writing SSE; modify the blocks that call
json.Marshal(history) and json.Marshal(evt) to capture the error, log it (e.g.,
using the request/handler logger or log.Printf) and skip sending that SSE
payload (or send a safe error event) instead of writing malformed/null data to
w; ensure you still call flusher.Flush() appropriately when you do send events
or when ending the stream, and reference the existing variables json.Marshal,
history, ch, evt, fmt.Fprintf, w and flusher to locate and update the
serialization + send logic.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/strategy/agent-orchestration-patterns.md`:
- Line 362: The concurrent appends to run.Tasks from goroutines in RunParallel
(each calling runAgentWithTracking and doing run.Tasks = append(run.Tasks,
*record)) introduce a race; protect mutations by synchronizing access—add a
mutex (e.g., a field on the Run struct like mu sync.Mutex) or use a dedicated
results channel and a single collector goroutine to append to run.Tasks; update
the goroutines to either lock/unlock the mutex around the append to run.Tasks or
send the record over the channel and perform the append from the collector to
eliminate the race (also apply the same fix to the similar code at the other
location around lines 465-467).
- Around line 539-557: The fenced code block that contains the ASCII flow
diagram (starting with the lines "sourcing", "gating", "profitability", etc.) is
missing a language specifier and triggers MD040; change the opening fence from
``` to ```text (or another appropriate language like ```mermaid if you intend
rendering) so the block is labeled — update the fence surrounding the diagram in
the same block where "sourcing" through "reviewer" appear.
- Around line 339-356: The code mutates record.Status, record.Output,
record.TokensUsed, record.CompletedAt and record.DurationMs directly after
calling record.Transition; instead, use record.Transition consistently to change
lifecycle state and let it handle timestamp/duration bookkeeping. Replace the
direct assignments with calls to record.Transition(TaskStatusFailed) (and set
record.Error) or record.Transition(TaskStatusCompleted) and then only set
non-lifecycle fields (e.g., record.Output and record.TokensUsed) if Transition
does not already populate them; remove manual setting of record.CompletedAt and
record.DurationMs and rely on Transition to set those based on record.StartedAt.
- Around line 333-360: PreRun and PostRun are treated like single functions but
Pattern 7 defines them as slices; change the calls to iterate over
def.Hooks.PreRun and def.Hooks.PostRun (e.g., for _, h := range def.Hooks.PreRun
{ if err := h(ctx, record); err != nil { ... } }) and handle returned errors
instead of ignoring them: for PreRun return the error after marking the record
failed (Transition(TaskStatusFailed), set record.Error) so the agent run doesn't
proceed; for PostRun iterate all hooks, capture/log any errors or attach them to
record.Error without dropping the original output; update references to
def.Hooks.PreRun and def.Hooks.PostRun and ensure the PostRun signature receives
(ctx, record, output) when invoking each hook.

In `@internal/api/handler/assessment_handler.go`:
- Around line 93-99: Currently GetProfile and GetFingerprint calls ignore
returned errors which can hide DB/timeout failures and incorrectly return
"pending"; update the block around h.assessment.GetProfile and
h.assessment.GetFingerprint to capture their errors, distinguish "not found"
(treat as nil profile/fingerprint and keep status "pending") from other errors
(log and return an appropriate error HTTP response, e.g., 500), and use the
error values from those calls when deciding whether to proceed to compute status
from profile.AssessmentStatus.

---

Nitpick comments:
In `@internal/api/handler/assessment_handler.go`:
- Around line 29-33: The handler currently just calls r.Body.Close() which can
leave unconsumed bytes on keep-alive connections; before closing r.Body (the
request body used in the handler where the snippet lives) call
io.Copy(io.Discard, r.Body) to fully drain it and then close; handle or ignore
the returned error appropriately (e.g., log with the existing logger or discard)
to avoid resource leaks and ensure proper connection reuse.
- Around line 444-463: The code currently ignores errors from json.Marshal when
serializing both the catch-up history and each evt before writing SSE; modify
the blocks that call json.Marshal(history) and json.Marshal(evt) to capture the
error, log it (e.g., using the request/handler logger or log.Printf) and skip
sending that SSE payload (or send a safe error event) instead of writing
malformed/null data to w; ensure you still call flusher.Flush() appropriately
when you do send events or when ending the stream, and reference the existing
variables json.Marshal, history, ch, evt, fmt.Fprintf, w and flusher to locate
and update the serialization + send logic.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 41cfcafd-791e-4cbf-9016-2385d7e47124

📥 Commits

Reviewing files that changed from the base of the PR and between 8759726 and 264c9c1.

📒 Files selected for processing (2)

• docs/strategy/agent-orchestration-patterns.md
• internal/api/handler/assessment_handler.go

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Hook execution snippet is incompatible with the hook type definitions.

Line 333 and Line 358 call hooks as single functions, but Pattern 7 defines PreRun and PostRun as slices. This would fail when implemented and also drops hook error handling.

Proposed doc/code-snippet correction

-    if def.Hooks != nil && def.Hooks.PreRun != nil {
-        if err := def.Hooks.PreRun(ctx, record); err != nil {
-            return nil, record, err
-        }
-    }
+    if def.Hooks != nil {
+        for _, hook := range def.Hooks.PreRun {
+            if err := hook(ctx, record); err != nil {
+                return nil, record, err
+            }
+        }
+    }
...
-    if def.Hooks != nil && def.Hooks.PostRun != nil {
-        def.Hooks.PostRun(ctx, record, output)
-    }
+    if def.Hooks != nil {
+        for _, hook := range def.Hooks.PostRun {
+            if hookErr := hook(ctx, record, output); hookErr != nil {
+                // decide: log and continue OR return hookErr
+            }
+        }
+    }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if def.Hooks != nil && def.Hooks.PreRun != nil {
	if err := def.Hooks.PreRun(ctx, record); err != nil {
	return nil, record, err
	}
	}
	record.Transition(TaskStatusRunning)
	output, err := o.agentRuntime.RunAgent(ctx, task)
	if err != nil {
	record.Transition(TaskStatusFailed)
	errStr := err.Error()
	record.Error = &errStr
	} else {
	record.Status = TaskStatusCompleted
	record.Output = output.Structured
	record.TokensUsed = output.TokensUsed
	}
	now := time.Now()
	record.CompletedAt = &now
	record.DurationMs = now.Sub(record.StartedAt).Milliseconds()
	// Execute hooks
	if def.Hooks != nil && def.Hooks.PostRun != nil {
	def.Hooks.PostRun(ctx, record, output)
	}
	if def.Hooks != nil {
	for _, hook := range def.Hooks.PreRun {
	if err := hook(ctx, record); err != nil {
	return nil, record, err
	}
	}
	}
	record.Transition(TaskStatusRunning)
	output, err := o.agentRuntime.RunAgent(ctx, task)
	if err != nil {
	record.Transition(TaskStatusFailed)
	errStr := err.Error()
	record.Error = &errStr
	} else {
	record.Status = TaskStatusCompleted
	record.Output = output.Structured
	record.TokensUsed = output.TokensUsed
	}
	now := time.Now()
	record.CompletedAt = &now
	record.DurationMs = now.Sub(record.StartedAt).Milliseconds()
	// Execute hooks
	if def.Hooks != nil {
	for _, hook := range def.Hooks.PostRun {
	if hookErr := hook(ctx, record, output); hookErr != nil {
	// decide: log and continue OR return hookErr
	}
	}
	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/strategy/agent-orchestration-patterns.md` around lines 333 - 360, PreRun
and PostRun are treated like single functions but Pattern 7 defines them as
slices; change the calls to iterate over def.Hooks.PreRun and def.Hooks.PostRun
(e.g., for _, h := range def.Hooks.PreRun { if err := h(ctx, record); err != nil
{ ... } }) and handle returned errors instead of ignoring them: for PreRun
return the error after marking the record failed (Transition(TaskStatusFailed),
set record.Error) so the agent run doesn't proceed; for PostRun iterate all
hooks, capture/log any errors or attach them to record.Error without dropping
the original output; update references to def.Hooks.PreRun and def.Hooks.PostRun
and ensure the PostRun signature receives (ctx, record, output) when invoking
each hook.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Use Transition(...) consistently instead of direct status/timestamp mutation.

Line 348 and Lines 353-355 manually set status/completion fields after already using Transition(...). This bypasses state rules and duplicates lifecycle bookkeeping.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/strategy/agent-orchestration-patterns.md` around lines 339 - 356, The
code mutates record.Status, record.Output, record.TokensUsed, record.CompletedAt
and record.DurationMs directly after calling record.Transition; instead, use
record.Transition consistently to change lifecycle state and let it handle
timestamp/duration bookkeeping. Replace the direct assignments with calls to
record.Transition(TaskStatusFailed) (and set record.Error) or
record.Transition(TaskStatusCompleted) and then only set non-lifecycle fields
(e.g., record.Output and record.TokensUsed) if Transition does not already
populate them; remove manual setting of record.CompletedAt and record.DurationMs
and rely on Transition to set those based on record.StartedAt.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Parallel design introduces a race on run.Tasks.

RunParallel executes runAgentWithTracking concurrently, and each goroutine appends to run.Tasks (Line 362). That shared slice mutation is unsafe without synchronization.

Proposed doc/code-snippet correction

 type PipelineRun struct {
+    mu sync.Mutex
     Tasks           []AgentTaskRecord `json:"tasks"`
 }

 ...
-    run.Tasks = append(run.Tasks, *record)
+    run.mu.Lock()
+    run.Tasks = append(run.Tasks, *record)
+    run.mu.Unlock()

Also applies to: 465-467

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/strategy/agent-orchestration-patterns.md` at line 362, The concurrent
appends to run.Tasks from goroutines in RunParallel (each calling
runAgentWithTracking and doing run.Tasks = append(run.Tasks, *record)) introduce
a race; protect mutations by synchronizing access—add a mutex (e.g., a field on
the Run struct like mu sync.Mutex) or use a dedicated results channel and a
single collector goroutine to append to run.Tasks; update the goroutines to
either lock/unlock the mutex around the append to run.Tasks or send the record
over the channel and perform the append from the collector to eliminate the race
(also apply the same fix to the similar code at the other location around lines
465-467).

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add a language to this fenced code block.

Line 539 starts a fenced block without a language, which will keep markdown lint failing (MD040).

🧰 Tools

🪛 markdownlint-cli2 (0.22.0)

[warning] 539-539: Fenced code blocks should have a language specified

(MD040, fenced-code-language)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/strategy/agent-orchestration-patterns.md` around lines 539 - 557, The
fenced code block that contains the ASCII flow diagram (starting with the lines
"sourcing", "gating", "profitability", etc.) is missing a language specifier and
triggers MD040; change the opening fence from ``` to ```text (or another
appropriate language like ```mermaid if you intend rendering) so the block is
labeled — update the fence surrounding the diagram in the same block where
"sourcing" through "reviewer" appear.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Silent error handling may mask infrastructure failures.

Both GetProfile and GetFingerprint errors are discarded. While a missing profile/fingerprint is expected (returns "pending"), a database connection error or timeout would also be silently ignored, returning a misleading "pending" status instead of an error response.

🛡️ Suggested improvement to distinguish missing vs failed

-	profile, _ := h.assessment.GetProfile(r.Context(), ac.TenantID)
-	fingerprint, _ := h.assessment.GetFingerprint(r.Context(), ac.TenantID)
+	profile, err := h.assessment.GetProfile(r.Context(), ac.TenantID)
+	if err != nil && !errors.Is(err, service.ErrNotFound) {
+		response.Error(w, http.StatusInternalServerError, "failed to fetch profile")
+		return
+	}
+	fingerprint, err := h.assessment.GetFingerprint(r.Context(), ac.TenantID)
+	if err != nil && !errors.Is(err, service.ErrNotFound) {
+		response.Error(w, http.StatusInternalServerError, "failed to fetch fingerprint")
+		return
+	}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@internal/api/handler/assessment_handler.go` around lines 93 - 99, Currently
GetProfile and GetFingerprint calls ignore returned errors which can hide
DB/timeout failures and incorrectly return "pending"; update the block around
h.assessment.GetProfile and h.assessment.GetFingerprint to capture their errors,
distinguish "not found" (treat as nil profile/fingerprint and keep status
"pending") from other errors (log and return an appropriate error HTTP response,
e.g., 500), and use the error values from those calls when deciding whether to
proceed to compute status from profile.AssessmentStatus.