feat: AI concierge — shared catalog, credits, assessment, strategy, suggestions (Phases 0-4)#2
Merged
Merged
Conversation
Platform-wide product catalog shared across all tenants. Product data (ASIN, title, BSR, price) is universal. Tenant-specific data (eligibility, margins) stays private per tenant. Every scan enriches the shared catalog for all future tenants. Credit model (like Apollo): - Cached products (< 24h): 0 credits (free) - Fresh SP-API calls: 1 credit each - Tiers: Free (500/mo), Starter (5K), Growth (25K), Scale (100K) New tables: - product_catalog (shared, keyed by ASIN) - brand_catalog (shared, with gating metadata) - tenant_product_eligibility (per-tenant, private) - tenant_product_margins (per-tenant, private) - credit_accounts + credit_transactions (billing) Services: - CreditService: balance, spend, grant, tier management - SharedCatalogService: credit-aware enrichment + eligibility checking API: GET /credits, GET /credits/transactions Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
100% coverage on credit_service.go and shared_catalog_service.go. Credit service tests (48): - Balance, spend, insufficient, monthly reset, tier upgrade - Transaction recording, SpendIfAvailable, full lifecycle - Error paths for all repo operations Shared catalog service tests (41): - EnrichProduct: cached/stale/no-credits/SP-API error paths - CheckEligibility: cached/fresh/stale/restricted paths - RecordFromScan: batch upsert, brand catalog, margin calculation - Credit-aware: 0 credits for cached, 1 credit for fresh API calls Domain type tests (11): - CreditTier.MonthlyCredits for all tiers + unknown - CreditAccount.Remaining edge cases (overdrawn, zero) - SharedProduct.IsFresh with nil/fresh/stale timestamps Total: 168 tests across all packages (was 72, added 96). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Account assessment scans 300 ASINs across 30 categories to build an
eligibility fingerprint for each seller. Classifies archetype
(greenhorn, ra_to_wholesale, expanding_pro, capital_rich) based on
account age, listing count, and stated capital.
Domain types:
- SellerProfile with archetype classification
- EligibilityFingerprint with category scores + brand probe results
- AssessmentProbe for the curated ASIN sample set
- ClassifyArchetype decision tree
Assessment samples:
- 30 categories in 3 tiers (high-volume, gated, niche)
- 25 top wholesale brands for brand-specific probes
- Stratified probing (top/mid/generic brands per category)
- Dynamic probe generation via ASINFinder pattern
Database:
- seller_profiles, eligibility_fingerprints
- category_eligibilities, assessment_probe_results
Services:
- AssessmentService: StartAssessment, RunEligibilityScan,
CompleteAssessment, GetProfile, GetFingerprint
- Integrates with SharedCatalogService for tenant eligibility caching
API: POST /assessment/start, GET /assessment/status,
GET /assessment/profile
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Immutable strategy versions with full audit trail. Every change creates
a new version. Users can rollback to any previous version (creates a
copy, never mutates history).
Domain types:
- StrategyVersion: versioned snapshot with goals, search params, status
- StrategyGoal: revenue/profit target with timeframe (measurable, time-bound)
- StrategySearchParams: per-goal discovery parameters (eligible categories/brands)
- StrategyStatus: draft → active → archived | rolled_back
- CategoryRecommendation, UngatingRecommendation
Strategy generation based on archetype:
- Greenhorn: $2K revenue in 90 days (longer for ungating)
- RA-to-Wholesale: $5K revenue in 30 days
- Expanding Pro: $3K profit in 14 days
- Capital-Rich: $10K revenue in 60 days
Search params built from eligibility fingerprint — only categories
with >30% open rate included. Eligible brands from probe results.
Rollback flow: current → rolled_back, create new version copying
target's params → active. Append-only history, fully auditable.
API: GET /strategy, GET /strategy/versions,
GET /strategy/versions/:id,
POST /strategy/versions/:id/activate,
POST /strategy/versions/:id/rollback
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Assessment service tests (28): - StartAssessment: archetype classification for all 4 types - RunEligibilityScan: eligibility counts, category open rates, SP-API error handling, confidence scaling, tenant eligibility storage - CompleteAssessment/FailAssessment status transitions Strategy service tests (31): - GenerateInitialStrategy: archetype-based goals (greenhorn=90d/$2K, ra_to_wholesale=30d/$5K, expanding_pro=14d/$3K, capital_rich=60d/$10K) - Search params from fingerprint (>30% open rate filter) - ActivateVersion: draft→active, rejects non-draft, archives previous - RollbackToVersion: copies params, marks current rolled_back, correct parent_version_id, version number increments Domain type tests (22): - ClassifyArchetype: all archetypes + boundary cases - StrategyGoal: DaysRemaining, ProgressPct edge cases Bug fixed: nil-pointer in RollbackToVersion when no active version exists. Total: 249 tests (was 168, added 81). All services >96% coverage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Discovery engine runs daily per tenant, directed by the active strategy.
Products found are presented as suggestions (not deals) — user must
accept to create a deal. Dismissals do NOT bias future recommendations.
DiscoverySuggestion: ASIN, title, brand, category, margin, reason,
status (pending/accepted/dismissed), linked to strategy version + goal.
DiscoveryQueueService:
- RunDailyDiscovery: loads strategy → searches eligible categories →
runs funnel with strategy params → creates up to 20 suggestions/day
- AcceptSuggestion/DismissSuggestion: user action endpoints
- Daily cap of 20 suggestions per tenant
API: GET /suggestions, GET /suggestions/all,
POST /suggestions/:id/accept, POST /suggestions/:id/dismiss
Tests (20): daily cap, strategy loading, funnel integration, accept/
dismiss, multi-category search, error handling. >90% coverage.
Total: 269 tests across all packages.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The "Wealthfront moment" — connect account, get assessed, see personalized strategy, approve with one click. Types + API + Hooks: - SellerProfile, EligibilityFingerprint, StrategyVersion, StrategyGoal, DiscoverySuggestion, CreditAccount types - 14 API client methods (assessment, strategy, suggestions, credits) - 4 hook files: use-assessment (polling), use-strategy (mutations), use-suggestions, use-credits Pages: - /onboarding: 4-step wizard (Connect → Discover → Reveal → Commit) with auto-advance on assessment completion, strategy approval - /strategy: active strategy card with goal progress bars, version history table with activate/rollback actions - /suggestions: pending suggestions feed with accept/dismiss per card Dashboard updates: - Credit balance card (tier, used/remaining, reset date) - Pending suggestions count - "Get Started" CTA linking to onboarding when no assessment exists Navigation: added Onboarding, Strategy, Suggestions nav items Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWires a shared product catalog and tenant credit system; adds seller assessment, strategy versioning, and a daily discovery suggestion pipeline across domain models, ports, Postgres migrations/repos, services, HTTP handlers, frontend pages/hooks/API client/types, and extensive unit tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SharedCatalogSvc as SharedCatalogService
participant Credits as CreditService
participant SPAPI as SP-API
participant CatalogRepo as SharedCatalogRepo
participant BrandRepo as BrandCatalogRepo
Client->>SharedCatalogSvc: EnrichProduct(ctx, tenantID, asin)
SharedCatalogSvc->>CatalogRepo: GetByASIN(asin)
alt cached & fresh
CatalogRepo-->>SharedCatalogSvc: product (fresh)
SharedCatalogSvc-->>Client: return cached product (no credit)
else stale or missing
SharedCatalogSvc->>Credits: SpendIfAvailable(tenantID, 1, CreditActionEnrichment)
alt credit granted
Credits-->>SharedCatalogSvc: true
SharedCatalogSvc->>SPAPI: GetProductDetails(asin)
alt SPAPI success
SPAPI-->>SharedCatalogSvc: product details
SharedCatalogSvc->>CatalogRepo: UpsertProduct(product)
SharedCatalogSvc->>BrandRepo: Upsert(brand)
SharedCatalogSvc-->>Client: enriched product (spent credit)
else SPAPI error
SPAPI-->>SharedCatalogSvc: error
SharedCatalogSvc-->>Client: stale product or nil
end
else credit unavailable
Credits-->>SharedCatalogSvc: false
SharedCatalogSvc-->>Client: stale product or nil
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (32)
apps/api/main.go-166-166 (1)
166-166:⚠️ Potential issue | 🟠 MajorDon’t ignore the bootstrap credit-account failure.
If
EnsureAccountfails here, startup continues with the default tenant half-initialized and the failure is invisible. Handle the error explicitly.Suggested fix
- creditSvc.EnsureAccount(ctx, defaultTenantID, domain.CreditTierFree) + if err := creditSvc.EnsureAccount(ctx, defaultTenantID, domain.CreditTierFree); err != nil { + slog.Error("failed to ensure default tenant credit account", "tenant_id", defaultTenantID, "error", err) + os.Exit(1) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/main.go` at line 166, The call to creditSvc.EnsureAccount(defaultTenantID, domain.CreditTierFree) is ignoring its error; change it to capture the returned error, check if err != nil, and handle it by either returning the error from the bootstrap function (propagate up) or logging a clear fatal error and exiting (e.g., log.Fatalf or os.Exit(1)) so startup does not continue with a half-initialized tenant; update the EnsureAccount invocation site to use err := creditSvc.EnsureAccount(ctx, defaultTenantID, domain.CreditTierFree) and handle err accordingly.internal/adapter/postgres/migrations/012_seller_profiles.sql-29-55 (1)
29-55:⚠️ Potential issue | 🟠 MajorPrevent duplicate rows per fingerprint.
internal/adapter/postgres/eligibility_fingerprint_repo.go:78-102inserts category and probe rows with plainINSERTs. Sinceeligibility_fingerprintsis unique per tenant, a rerun can append duplicate(fingerprint_id, category)and(fingerprint_id, asin)rows and skew later reads. Add uniqueness here or switch the writes to upserts.Suggested schema guardrails
CREATE TABLE IF NOT EXISTS category_eligibilities ( id UUID PRIMARY KEY DEFAULT gen_random_uuid(), fingerprint_id UUID NOT NULL, tenant_id UUID NOT NULL, category TEXT NOT NULL, probe_count INT NOT NULL DEFAULT 0, open_count INT NOT NULL DEFAULT 0, gated_count INT NOT NULL DEFAULT 0, - open_rate NUMERIC(5,2) NOT NULL DEFAULT 0 + open_rate NUMERIC(5,2) NOT NULL DEFAULT 0, + UNIQUE (fingerprint_id, category) ); @@ CREATE TABLE IF NOT EXISTS assessment_probe_results ( id UUID PRIMARY KEY DEFAULT gen_random_uuid(), fingerprint_id UUID NOT NULL, tenant_id UUID NOT NULL, asin TEXT NOT NULL, brand TEXT NOT NULL DEFAULT '', category TEXT NOT NULL DEFAULT '', tier TEXT NOT NULL DEFAULT '', eligible BOOLEAN NOT NULL, reason TEXT NOT NULL DEFAULT '', - checked_at TIMESTAMPTZ NOT NULL DEFAULT now() + checked_at TIMESTAMPTZ NOT NULL DEFAULT now(), + UNIQUE (fingerprint_id, asin) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/postgres/migrations/012_seller_profiles.sql` around lines 29 - 55, Add uniqueness guarantees for category_eligibilities and assessment_probe_results to prevent duplicate (fingerprint_id, category) and (fingerprint_id, asin) rows: add UNIQUE constraints or unique indexes on (fingerprint_id, category) for category_eligibilities and on (fingerprint_id, asin) for assessment_probe_results, and/or change the plain INSERTs in internal/adapter/postgres/eligibility_fingerprint_repo.go (the insert logic around lines 78-102) to use INSERT ... ON CONFLICT DO UPDATE/DO NOTHING so writes become upserts referencing the same unique keys.apps/web/src/app/(app)/dashboard/page.tsx-53-55 (1)
53-55:⚠️ Potential issue | 🟠 MajorAvoid nesting interactive elements in the onboarding CTA.
Linkrenders an anchor, so wrappingButtoncreates nested interactive controls. That breaks keyboard and screen-reader behavior. Render a single interactive element instead.Suggested fix
- <Link href="/onboarding"> - <Button>Get Started</Button> - </Link> + <Button asChild> + <Link href="/onboarding">Get Started</Link> + </Button>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(app)/dashboard/page.tsx around lines 53 - 55, The CTA currently nests a Button inside a Link (Link + Button), creating nested interactive controls; change it so only one interactive element is rendered: either make the Link the single interactive element and style it to look like the Button (replace <Link><Button>...</Button></Link> with <Link className="button-styles">Get Started</Link>), or remove the Link and make the Button perform navigation via router.push (useNextRouter useRouter().push('/onboarding') in the Button's onClick). Update the Dashboard CTA to use one of these approaches and keep the visual styling identical to the current Button.apps/web/src/app/(app)/strategy/page.tsx-114-124 (1)
114-124:⚠️ Potential issue | 🟠 MajorVersion history is reading the wrong version property.
StrategyVersionusesversion_numberinapps/web/src/lib/types.ts, sov.versionwill rendervundefinedfor every row.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(app)/strategy/page.tsx around lines 114 - 124, The table is rendering the wrong property for StrategyVersion: replace references to v.version with the correct property v.version_number (used by the StrategyVersion type) in the versions.map render block (the <tr> mapping that currently references v.id, v.version, v.status, v.change_reason, v.created_by, v.created_at) so the version column shows v.version_number instead of undefined; update any other uses in that map that assume v.version to use v.version_number.apps/web/src/app/(app)/strategy/page.tsx-65-77 (1)
65-77:⚠️ Potential issue | 🟠 MajorUse the actual
StrategyGoalfields in the active card.
apps/web/src/lib/types.tsdefinesStrategyGoalwithtimeframe_endandcurrent_progress. Renderinggoal.timeframeandgoal.progresshere turns the countdown intoNaN days remainingand keeps progress at0%even when the backend returned real values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(app)/strategy/page.tsx around lines 65 - 77, The active goal card is using wrong field names (goal.timeframe and goal.progress) so daysRemaining returns NaN and progress stays 0; update the rendering to use the actual StrategyGoal fields: replace goal.timeframe with goal.timeframe_end when calling daysRemaining and replace goal.progress with goal.current_progress when passing to ProgressPct, and ensure the mapped item is typed as StrategyGoal (or cast) so TypeScript picks up the correct fields.apps/web/src/app/(app)/strategy/page.tsx-126-143 (1)
126-143:⚠️ Potential issue | 🟠 MajorBlock both version actions while one mutation is running.
These buttons only disable against their own mutation flag. A user can click Activate and then Rollback before invalidation finishes, sending conflicting writes for the same version. Disable the pair when either mutation is pending, ideally scoped to
v.id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(app)/strategy/page.tsx around lines 126 - 143, Both buttons use only their own pending flag so users can trigger conflicting Activate and Rollback requests; update the disable logic to block both buttons whenever either mutation is pending for the same version id. In the UI around the Button pair referencing activateVersion, rollbackVersion and v.id, compute a single boolean (e.g., isMutatingVersion = activateVersion.isPendingFor?.(v.id) || rollbackVersion.isPendingFor?.(v.id) or, if your mutation API exposes last variables, compare activateVersion.isPending && activateVersion.variables === v.id || rollbackVersion.isPending && rollbackVersion.variables === v.id) and pass that value to both Button disabled props so the pair is disabled while either mutation for v.id is running.apps/web/src/app/(app)/suggestions/page.tsx-51-58 (1)
51-58:⚠️ Potential issue | 🟠 MajorRead the fields the API actually returns.
apiClient.getPendingSuggestions()returnsDiscoverySuggestion[], andapps/web/src/lib/types.tsdefines the price/margin fields asbuy_box_priceandestimated_margin_pct.suggestion.priceandsuggestion.margin_pctrenderundefinedhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(app)/suggestions/page.tsx around lines 51 - 58, The UI is accessing nonexistent fields suggestion.price and suggestion.margin_pct, causing undefined values; update the rendering in page.tsx to use the API's actual fields from DiscoverySuggestion returned by getPendingSuggestions(), namely suggestion.buy_box_price (format with toFixed(2)) and suggestion.estimated_margin_pct (format with toFixed(1) and append '%'), and keep null/undefined-safe rendering (e.g., conditional or fallback) to avoid runtime errors.apps/web/src/app/(app)/onboarding/page.tsx-217-223 (1)
217-223:⚠️ Potential issue | 🟠 MajorRender the strategy timeframe from the actual goal model.
StrategyGoalinapps/web/src/lib/types.ts:234-243hastimeframe_startandtimeframe_end;goal.timeframeis undefined here, so the recommendation text is broken.🤖 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 217 - 223, The JSX rendering of each goal uses a non-existent property goal.timeframe causing broken text; update the recommendation line inside the draftVersion.goals.map (the block rendering goal.type and target) to use the StrategyGoal fields timeframe_start and timeframe_end instead (e.g., render a formatted range like `${goal.timeframe_start} - ${goal.timeframe_end}` or apply toLocaleDateString if they are Date strings) while keeping the existing target_amount formatting (goal.target_amount?.toLocaleString()) and the same element structure.apps/web/src/app/(app)/suggestions/page.tsx-77-92 (1)
77-92:⚠️ Potential issue | 🟠 MajorDisable both card actions once one write starts.
After Accept begins, Dismiss stays clickable until its own mutation starts, so the same suggestion can receive both POSTs. Gate both buttons on
acceptSuggestion.isPending || dismissSuggestion.isPending, ideally tied to the currentsuggestion.id.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/web/src/app/`(app)/suggestions/page.tsx around lines 77 - 92, The Accept/Dismiss buttons currently only disable based on their own mutation's isPending, allowing both POSTs for the same suggestion; update the logic so both buttons are disabled when either action is pending for that specific suggestion (use acceptSuggestion and dismissSuggestion state together). Tie the pending check to the suggestion id: either extend the mutation tracking to expose per-id pending status (e.g., isPendingFor(suggestion.id)) or add a local pendingSuggestionId state set to suggestion.id when calling acceptSuggestion.mutate(...) or dismissSuggestion.mutate(...) and cleared on settle, then use (acceptPendingForId || dismissPendingForId) to disable both Button components for that suggestion.apps/web/src/app/(app)/onboarding/page.tsx-29-33 (1)
29-33:⚠️ Potential issue | 🟠 MajorAdd a failed-assessment path to the discover step.
useAssessmentStatus()stops polling once the status becomes"failed"inapps/web/src/hooks/use-assessment.ts:19-29, but this component only advances on"completed". A failed run leaves the user stuck on this progress card with no retry or error message.Also applies to: 125-147
🤖 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 29 - 33, The discover step currently only advances on assessment.status === "completed", leaving the UI stuck if the assessment becomes "failed"; update the useEffect that watches step and assessment?.status (and the similar block at lines ~125-147) to also handle assessment?.status === "failed" by advancing the step to a failure/ retry route or setting an error state so the UI can show a retry/error card; specifically modify the effect that checks (step === "discover") to call setStep("discoverFailed") or set an error flag (or call a provided showError/enableRetry handler) when assessment?.status === "failed", and ensure the component renders the corresponding failure UI for that step so users can retry or see the error.apps/web/src/app/(app)/onboarding/page.tsx-180-198 (1)
180-198:⚠️ Potential issue | 🟠 MajorThe category table is using the wrong fingerprint fields.
CategoryEligibilityinapps/web/src/lib/types.ts:212-218exposescategory,probe_count,open_count,gated_count, andopen_rate.cat.name/cat.eligiblewill render blank category names and treat every row as rejected.🤖 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 180 - 198, The table is referencing non-existent fields cat.name and cat.eligible; update the rendering to use the CategoryEligibility shape: replace cat.name with cat.category for the label and compute eligibility from the real fields (e.g., derive a boolean like const isEligible = cat.gated_count === 0 (or whichever business rule applies using probe_count/gated_count/open_count)) then pass isEligible to StatusPill (StatusPill status={isEligible ? "approved" : "rejected"}); keep the open-rate rendering as (cat.open_rate * 100).toFixed(1)%. Ensure you reference CategoryEligibility in types and the table rows in page.tsx where map((cat: any) => ...) is used.internal/port/strategy.go-15-16 (1)
15-16:⚠️ Potential issue | 🟠 MajorKeep tenant scoping on status writes.
SetStatusis the only mutating repo method that dropstenantID. If an ID from another tenant is passed through, the adapter has no interface-level way to enforce ownership on the update. Include the tenant in this contract, likeGetByIDandActivate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/port/strategy.go` around lines 15 - 16, The SetStatus method currently omits tenant scoping which allows cross-tenant updates; change the repository interface signature SetStatus(ctx context.Context, tenantID domain.TenantID, id domain.StrategyVersionID, status domain.StrategyStatus) error to mirror GetByID and Activate, update all adapter/implementation functions that implement SetStatus (and any callers) to accept and pass the tenantID, and ensure the implementation uses tenantID when executing the update to enforce ownership checks.apps/web/src/app/(app)/onboarding/page.tsx-21-26 (1)
21-26:⚠️ Potential issue | 🟠 MajorInvalidate strategy versions cache when assessment completes.
useStartAssessment()only invalidatesassessment.statusandassessment.profile(seeapps/web/src/hooks/use-assessment.ts), but does not invalidate thestrategy.versionscache. When the assessment completes and the component auto-advances to the reveal step (lines 29-33),useStrategyVersions()returns stale data. Since the backend creates the draft version during the assessment, but the client cache is never refreshed,draftVersionat line 26 remains undefined, leaving the Reveal card without a draft strategy and making the Approve button a no-op.Add invalidation of
queryKeys.strategy.versionsto theonSuccesshandler inuseStartAssessment().🤖 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 21 - 26, The startAssessment hook's onSuccess currently invalidates assessment.status and assessment.profile but not the strategy versions, causing useStrategyVersions() to return stale data; update the onSuccess handler inside useStartAssessment() to also invalidate queryKeys.strategy.versions (the key used by useStrategyVersions) so that after the assessment completes the client refetches versions and draftVersion (used where useAssessmentStatus, useStrategyVersions and draftVersion are referenced) becomes available.internal/api/handler/assessment_handler.go-42-42 (1)
42-42:⚠️ Potential issue | 🟠 MajorApply nil check consistently in
GetStatusandGetProfile.Both methods access
ac.TenantIDwithout verifyingacis non-nil.Also applies to: 57-57
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler/assessment_handler.go` at line 42, GetStatus and GetProfile call middleware.GetAuthContext(r.Context()) into variable ac and then dereference ac.TenantID without checking for nil; add a nil-check after calling middleware.GetAuthContext in both GetStatus and GetProfile (and any other places that use ac) to handle the absent auth context: if ac == nil return an appropriate HTTP error (e.g., 401/400) or log and respond, avoiding a panic, and ensure all subsequent uses of ac.TenantID in those functions assume ac is non-nil.internal/api/handler/suggestion_handler.go-22-23 (1)
22-23:⚠️ Potential issue | 🟠 MajorPotential nil-pointer dereference on
acin multiple handlers.
middleware.GetAuthContextcan returnnil. Add nil checks inListPending,ListAll, andAcceptbefore accessingac.TenantID.Also applies to: 32-33, 42-43
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler/suggestion_handler.go` around lines 22 - 23, In SuggestionHandler methods ListPending, ListAll, and Accept, guard against middleware.GetAuthContext returning nil before reading ac.TenantID: check if ac == nil and return an appropriate HTTP 401/400 response (or call h.writeError/h.sendError as used elsewhere) instead of dereferencing; update the code in the functions that call middleware.GetAuthContext (where ac is used) to perform this nil-check and short-circuit with a clear error message when nil to avoid a nil-pointer dereference.internal/api/handler/credit_handler.go-20-22 (1)
20-22:⚠️ Potential issue | 🟠 MajorPotential nil-pointer dereference on
ac.
middleware.GetAuthContextcan returnnilif the auth context is not set (seeinternal/api/middleware/auth.go:41-44). Accessingac.TenantIDwithout a nil check will cause a panic.🛡️ Proposed fix
func (h *CreditHandler) GetBalance(w http.ResponseWriter, r *http.Request) { ac := middleware.GetAuthContext(r.Context()) + if ac == nil { + response.Error(w, http.StatusUnauthorized, "unauthorized") + return + } account, err := h.credits.GetBalance(r.Context(), ac.TenantID)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler/credit_handler.go` around lines 20 - 22, middleware.GetAuthContext can return nil so accessing ac.TenantID may panic; update the handler to nil-check the returned ac before calling h.credits.GetBalance: after calling middleware.GetAuthContext(r.Context()), if ac is nil return an appropriate HTTP error (e.g., 401/unauthorized) or write an error response and stop processing, otherwise proceed to call h.credits.GetBalance(r.Context(), ac.TenantID); ensure any subsequent uses of ac in this function also assume non-nil only after that check.internal/api/handler/credit_handler.go-38-40 (1)
38-40:⚠️ Potential issue | 🟠 MajorSame nil-pointer risk in
GetTransactions.Apply the same nil check for
achere.🛡️ Proposed fix
func (h *CreditHandler) GetTransactions(w http.ResponseWriter, r *http.Request) { ac := middleware.GetAuthContext(r.Context()) + if ac == nil { + response.Error(w, http.StatusUnauthorized, "unauthorized") + return + } txs, err := h.credits.GetTransactions(r.Context(), ac.TenantID, 50)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler/credit_handler.go` around lines 38 - 40, The code calls h.credits.GetTransactions(r.Context(), ac.TenantID, 50) without ensuring ac from middleware.GetAuthContext(r.Context()) is non-nil; add a nil check after ac := middleware.GetAuthContext(r.Context()) (e.g., if ac == nil) and return an appropriate HTTP error/response (matching existing handlers' behavior) before calling h.credits.GetTransactions, referencing ac, middleware.GetAuthContext, and h.credits.GetTransactions to locate where to change.internal/api/handler/assessment_handler.go-21-22 (1)
21-22:⚠️ Potential issue | 🟠 MajorPotential nil-pointer dereference on
ac.Same issue as in
CreditHandler:middleware.GetAuthContextcan returnnil. Add a nil check before accessingac.TenantID.🛡️ Proposed fix
func (h *AssessmentHandler) Start(w http.ResponseWriter, r *http.Request) { ac := middleware.GetAuthContext(r.Context()) + if ac == nil { + response.Error(w, http.StatusUnauthorized, "unauthorized") + 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 21 - 22, The code calls middleware.GetAuthContext(r.Context()) and immediately uses ac.TenantID, which can cause a nil-pointer panic; modify the handler (the function where GetAuthContext is called, analogous to CreditHandler) to check if ac == nil and return an appropriate error/HTTP response (e.g., 401/400) before accessing ac.TenantID, ensuring you handle the nil case consistently with other handlers like CreditHandler.internal/port/suggestion.go-15-16 (1)
15-16:⚠️ Potential issue | 🟠 Major
AcceptandDismisslack tenant ID parameter — potential cross-tenant access.These methods accept only the suggestion ID, allowing operations on any tenant's suggestions if the ID is known or guessed. Per coding guidelines, tenant ID must be required in every database query. Add
tenantIDto enforce tenant-scoped mutations.🛡️ Proposed fix
- Accept(ctx context.Context, id domain.SuggestionID, dealID domain.DealID) error - Dismiss(ctx context.Context, id domain.SuggestionID) error + Accept(ctx context.Context, tenantID domain.TenantID, id domain.SuggestionID, dealID domain.DealID) error + Dismiss(ctx context.Context, tenantID domain.TenantID, id domain.SuggestionID) errorAs per coding guidelines: "Tenant ID must be required in every database query, with RLS as a safety net rather than primary mechanism."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/port/suggestion.go` around lines 15 - 16, Accept and Dismiss on the suggestion port accept only a SuggestionID (and DealID for Accept) which allows cross-tenant operations; update the method signatures to require a tenant ID parameter (e.g., add tenantID domain.TenantID) so tenant-scoped mutations are enforced, and update all implementations and callers of Accept(ctx, id, dealID) and Dismiss(ctx, id) to pass the tenantID as the first/appropriate argument and use it in DB queries.internal/service/credit_service.go-35-42 (1)
35-42:⚠️ Potential issue | 🟠 MajorA failed post-reset reload can return
nil, nil.The second
Geterror is discarded. If that reload fails,accountbecomes nil andGetBalancestill returns success, which can cascade into nil dereferences inHasCreditsandSpend.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/credit_service.go` around lines 35 - 42, The reload after ResetMonthly currently discards the error and can return a nil account; change the post-reset reload to capture and check the error (e.g., account, err := s.accounts.Get(ctx, tenantID)) and return the error if non-nil instead of proceeding; ensure the ResetMonthly + subsequent Get path in the method that checks account.ResetAt (and ultimately GetBalance/HasCredits/Spend callers) returns a non-nil account or an error so callers never receive (nil, nil).internal/adapter/postgres/eligibility_fingerprint_repo.go-48-58 (1)
48-58:⚠️ Potential issue | 🟠 MajorDon’t return partial fingerprints when detail loading fails.
Both detail queries ignore query errors, ignore
Scanerrors, and never checkrows.Err(). That can return a seemingly valid fingerprint with empty or truncatedCategories/BrandResults, which hides storage failures as success.Also applies to: 62-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/postgres/eligibility_fingerprint_repo.go` around lines 48 - 58, The category and brand detail queries (r.pool.Query -> catRows / brandRows) currently ignore Query errors, Scan errors, and rows.Err(), which can return partial fp results; change the logic in the Category and Brand loading sections (references: r.pool.Query, catRows, brandRows, domain.CategoryEligibility, fp.Categories, fp.BrandResults) to: check and return the Query error immediately if non-nil, defer Close only after a successful Query, check the error returned by each Scan and return it on failure, after the loop check rows.Err() and return it if non-nil, and propagate any such errors up instead of appending incomplete entries so that a failed detail load yields an overall error rather than a truncated fingerprint.internal/service/credit_service.go-86-90 (1)
86-90:⚠️ Potential issue | 🟠 Major
SpendIfAvailablehides outages as “no credits”.The doc says this should soften insufficient-balance cases, but it currently returns
falsefor every error path fromSpend, including repository failures. Callers can’t distinguish “user is out of credits” from “credit system is unavailable”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/credit_service.go` around lines 86 - 90, SpendIfAvailable currently swallows all errors from Spend and returns false for outages; change its signature to return (bool, error) and propagate non-insufficient-balance errors: call s.Spend(ctx, tenantID, amount, action, reference); if err == nil return true, nil; if errors.Is(err, domain.ErrInsufficientBalance) return false, nil; otherwise return false, err. Update callers accordingly to handle the propagated error. Ensure you import "errors" and reference SpendIfAvailable, Spend, and domain.ErrInsufficientBalance.internal/domain/seller_profile.go-44-57 (1)
44-57:⚠️ Potential issue | 🟠 Major
greenhornshort-circuits the documented listing-count rules.The first branch ignores
activeListings, so any seller younger than 90 days is classified asgreenhornunless they hit the capital-rich threshold. That does not match the decision tree in the comment (greenhornrequires<10listings, and>=50listings should qualify forexpanding_pro) and will mis-bucket fast-scaling accounts.🤖 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 44 - 57, In ClassifyArchetype, the branch for accountAgeDays < 90 ignores activeListings and misclassifies fast-scaling young sellers as greenhorn; update the logic to consider listing-count thresholds before or within the young-account branch: first handle expanding threshold (activeListings >= SellerArchetypeExpandingPro's cutoff, i.e., >=50) to return SellerArchetypeExpandingPro, then for age < 90 keep the statedCapital >= 50000 check to return SellerArchetypeCapitalRich, otherwise use activeListings to decide between SellerArchetypeGreenhorn (<10) and SellerArchetypeRAToWholesale (>=10); ensure overall ordering still respects the existing age <=365 && activeListings >=10 rule and the activeListings >=50 rule.internal/service/assessment_samples.go-122-129 (1)
122-129:⚠️ Potential issue | 🟠 Major
SPAPIASINFindersilently disables the production path.This constructor returns
nil, andGenerateProbeListimmediately returnsnilfor a nil finder. Any caller wiring the advertised production finder will generate no probes at all. Please back this with a real port-driven implementation or fail loudly instead of returning a nil finder.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/assessment_samples.go` around lines 122 - 129, SPAPIASINFinder currently returns nil which causes callers like GenerateProbeList to silently produce no probes; replace the nil return with a proper port-driven implementation that wraps the provided searcher (using searcher.SearchProducts) and implements the ASINFinder interface (or, if you can't implement the production behavior yet, return a non-nil "failing" ASINFinder implementation whose methods return a clear error indicating the SP-API integration is unimplemented). Ensure the returned implementation surfaces errors (do not panic in production flow) so GenerateProbeList callers can detect and handle the failure instead of getting a nil finder.internal/service/assessment_samples.go-79-119 (1)
79-119:⚠️ Potential issue | 🟠 MajorProbe generation tops out at 150, not 300.
This builder currently produces 90 category probes + 50 brand probes + 10 calibration probes. A “full” generated assessment therefore never reaches the 300-probe scan target described elsewhere, and any confidence logic keyed off 300 will underreport a complete run.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/assessment_samples.go` around lines 79 - 119, GenerateProbeList currently yields 150 probes (90 categories + 50 brands + 10 calibration) instead of the expected 300; fix GenerateProbeList by adjusting the probe counts so totals sum to 300: double the category probe output by multiplying the per-tier counts computed from ProbesPerTier(cat.Tier) (use topCount/midCount/genericCount logic but scale counts by 2), increase brand probes from 2 to 4 in the TopWholesaleBrands loop (finder(brand, "brand_probe", 4)), and increase calibration probes from 10 to 20 (finder("calibration","calibration",20)); keep using the same finder(...) calls and preserve the existing split logic for categories.internal/api/handler/strategy_handler.go-24-29 (1)
24-29:⚠️ Potential issue | 🟠 MajorDon’t collapse every service failure into
404.
GetActiveandGetVersiontreat any returned error as “not found”. That masks repository/timeouts as missing strategies and makes operational failures look like valid empty states. Please distinguish not-found from unexpected errors and return500for the latter.Also applies to: 47-50
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler/strategy_handler.go` around lines 24 - 29, The handler currently maps every error from h.strategy.GetActive and h.strategy.GetVersion to a 404; change both call sites (h.strategy.GetActive(...) and h.strategy.GetVersion(...)) to distinguish not-found errors from unexpected failures by using errors.Is/As against your repository's not-found sentinel (e.g., store.ErrNotFound or sql.ErrNoRows/Gorm's ErrRecordNotFound), return response.Error(..., http.StatusNotFound, "...") only for that case, and for all other errors log the error and return response.Error(..., http.StatusInternalServerError, "internal server error"). Ensure you update both the GetActive and GetVersion branches accordingly and include the underlying error in logging but not in the client-facing message.internal/api/handler/strategy_handler.go-59-60 (1)
59-60:⚠️ Potential issue | 🟠 MajorAvoid returning raw service errors to clients.
err.Error()here can expose wrapped internal details and makes the HTTP contract depend on service/repository message text. Map domain errors to stable client messages/status codes instead.Also applies to: 70-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler/strategy_handler.go` around lines 59 - 60, The handler currently returns raw service errors (err.Error()) from calls like h.strategy.ActivateVersion and h.strategy.DeactivateVersion which can leak internal details and couple HTTP responses to service text; replace these direct error strings with a mapping layer that inspects the returned error (using errors.Is, error types or sentinel errors from the domain/service) and translates them into stable HTTP status codes and user-facing messages before calling response.Error. Locate the ActivateVersion/DeactivateVersion calls in strategy_handler.go, detect expected domain errors (e.g., not found, validation, conflict, internal) and switch them to explicit responses (404 with "strategy version not found", 400 with "invalid request", 409 for conflicts, 500 with generic "internal server error") rather than returning err.Error(). Ensure the mapping covers wrapped errors and falls back to a generic 500/generic message for unknown errors.internal/adapter/postgres/eligibility_fingerprint_repo.go-19-30 (1)
19-30:⚠️ Potential issue | 🟠 MajorRescans can leave fingerprint details out of sync with the current snapshot.
Createkeeps a single parent row per tenant, butSaveProbeResultsandSaveCategoryEligibilitiesonly append child rows. After a second assessment, old detail rows are still present, so reads will mix historical and current probe/category data for the same tenant. Replace the child rows for the current snapshot in one transaction instead of doing append-only inserts here.Also applies to: 78-101
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/postgres/eligibility_fingerprint_repo.go` around lines 19 - 30, The Create method upserts a single parent row but leaves old child probe/category rows intact so rescans mix historical and current details; change Create (and similarly the logic in SaveProbeResults and SaveCategoryEligibilities) to perform the parent upsert and the child-row replacement inside a single transaction: begin a tx, upsert the eligibility_fingerprints parent (function EligibilityFingerprintRepo.Create), delete existing child probe and category rows for the same tenant and the same assessed snapshot (use tenant_id and assessed_at or the parent id as the filter), then insert the new probe and category child rows, and finally commit the tx so reads only see the replaced snapshot atomically. Ensure SaveProbeResults / SaveCategoryEligibilities also participate in or use the same transactional flow rather than append-only inserts.internal/adapter/postgres/strategy_version_repo.go-135-142 (1)
135-142:⚠️ Potential issue | 🟠 MajorArchive operation error is silently ignored, risking multiple active versions.
If archiving the current active version fails, the method proceeds to activate the new version anyway. This could leave the tenant with multiple "active" strategy versions in the database, violating the invariant that only one version should be active per tenant.
🐛 Proposed fix
func (r *StrategyVersionRepo) Activate(ctx context.Context, tenantID domain.TenantID, id domain.StrategyVersionID) error { // Archive current active - r.pool.Exec(ctx, ` + _, err := r.pool.Exec(ctx, ` UPDATE strategy_versions SET status = 'archived' WHERE tenant_id = $1 AND status = 'active' `, tenantID) + if err != nil { + return fmt.Errorf("archive current active: %w", err) + } // Activate new return r.SetStatus(ctx, id, domain.StrategyStatusActive) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/postgres/strategy_version_repo.go` around lines 135 - 142, The Archive UPDATE in StrategyVersionRepo.Activate currently ignores errors from r.pool.Exec and may proceed to call SetStatus, risking multiple active versions; change Activate to perform the archive and activation inside a single transactional flow: begin a transaction (using the pool/txn API you use elsewhere), execute the UPDATE to set existing active rows to 'archived' and check for Exec errors (and rows affected if needed), then call SetStatus (or an internal tx-safe equivalent) within the same transaction to set the new version to active, and commit; on any error ensure you rollback and return the error instead of proceeding. Ensure you reference and update the calls around r.pool.Exec and SetStatus in StrategyVersionRepo.Activate so both operations run atomically and errors are propagated.internal/adapter/postgres/strategy_version_repo.go-119-133 (1)
119-133:⚠️ Potential issue | 🟠 MajorMissing tenant_id filter in SetStatus query could allow cross-tenant status changes.
The
SetStatusmethod updates byidalone without filtering bytenant_id. While the calling code may ensure correct tenant ownership, this violates the coding guideline that tenant ID must be required in every database query.🐛 Proposed fix to add tenant_id filter
-func (r *StrategyVersionRepo) SetStatus(ctx context.Context, id domain.StrategyVersionID, status domain.StrategyStatus) error { +func (r *StrategyVersionRepo) SetStatus(ctx context.Context, tenantID domain.TenantID, id domain.StrategyVersionID, status domain.StrategyStatus) error { now := time.Now() var extra string switch status { case domain.StrategyStatusActive: extra = ", activated_at = $3" case domain.StrategyStatusRolledBack: extra = ", rolled_back_at = $3" default: - _, err := r.pool.Exec(ctx, `UPDATE strategy_versions SET status = $2 WHERE id = $1`, id, status) + _, err := r.pool.Exec(ctx, `UPDATE strategy_versions SET status = $2 WHERE id = $1 AND tenant_id = $3`, id, status, tenantID) return err } - _, err := r.pool.Exec(ctx, fmt.Sprintf(`UPDATE strategy_versions SET status = $2%s WHERE id = $1`, extra), id, status, now) + _, err := r.pool.Exec(ctx, fmt.Sprintf(`UPDATE strategy_versions SET status = $2%s WHERE id = $1 AND tenant_id = $4`, extra), id, status, now, tenantID) return err }Note: This requires updating the
port.StrategyVersionRepointerface and all callers to passtenantID.As per coding guidelines: "Tenant ID must be required in every database query, with RLS as a safety net rather than primary mechanism"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/postgres/strategy_version_repo.go` around lines 119 - 133, The SetStatus method in StrategyVersionRepo must include tenant_id in its UPDATE queries to prevent cross-tenant changes: change the SetStatus signature (and the port.StrategyVersionRepo interface and all callers) to accept tenantID, then update both SQL Exec calls in SetStatus to add "AND tenant_id = $N" to the WHERE clause and pass tenantID as a parameter (adjusting parameter order for id, tenantID, status, now as needed); ensure all callers are updated to provide the tenantID when calling StrategyVersionRepo.SetStatus.internal/service/discovery_queue_service.go-45-51 (1)
45-51:⚠️ Potential issue | 🟠 MajorCountToday error handling allows exceeding daily cap.
If
CountTodayfails (e.g., database unavailable), the error is only logged andtodayCountremains 0. This allows the discovery to proceed uncapped, potentially creating far more than 20 suggestions.Consider returning an error or using a conservative fallback (e.g., treat as cap reached) when the count cannot be verified.
🛡️ Proposed fix for safer error handling
todayCount, err := s.suggestions.CountToday(ctx, tenantID) if err != nil { slog.Warn("discovery-queue: failed to count today's suggestions", "error", err) + return nil, fmt.Errorf("check daily cap: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/discovery_queue_service.go` around lines 45 - 51, The current handling of s.suggestions.CountToday leaves todayCount at 0 on error and can bypass MaxDailySuggestions; instead, when CountToday returns an error, fail fast by returning that error (or at minimum treat it as the cap reached) so discovery cannot proceed uncapped. Update the block around CountToday to either return the error from CountToday (e.g., return nil, err) or set todayCount = MaxDailySuggestions and log the failure, referencing CountToday, todayCount, MaxDailySuggestions and s.suggestions so the change is applied in the correct spot.internal/service/assessment_service.go-126-135 (1)
126-135: 🛠️ Refactor suggestion | 🟠 MajorDirect access to
SharedCatalogService.eligibilitybreaks encapsulation.Line 134 directly accesses
s.sharedCatalog.eligibility.Set(...), bypassing the service layer. This couples the assessment service to the internal structure ofSharedCatalogService.Consider adding a public method to
SharedCatalogServicefor recording eligibility, or passport.TenantEligibilityRepodirectly toAssessmentService.♻️ Option 1: Add method to SharedCatalogService
In
shared_catalog_service.go:// RecordTenantEligibility stores a tenant's eligibility check result. func (s *SharedCatalogService) RecordTenantEligibility(ctx context.Context, e *domain.TenantEligibility) error { return s.eligibility.Set(ctx, e) }Then in
assessment_service.go:- s.sharedCatalog.eligibility.Set(ctx, te) + s.sharedCatalog.RecordTenantEligibility(ctx, te)🤖 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 126 - 135, The code directly accesses s.sharedCatalog.eligibility.Set(...) which breaks encapsulation; add a public method on SharedCatalogService (e.g., RecordTenantEligibility(ctx context.Context, e *domain.TenantEligibility) error) that calls s.eligibility.Set(ctx, e) and returns the error, then update AssessmentService to call s.sharedCatalog.RecordTenantEligibility(ctx, te) and handle the returned error; alternatively, inject port.TenantEligibilityRepo into AssessmentService and use that repo instead of reaching into SharedCatalogService.
🟡 Minor comments (10)
apps/api/main.go-116-124 (1)
116-124:⚠️ Potential issue | 🟡 MinorThe margins field is wired with
nilbut currently unused—consider removing or implementing.The 4th parameter of
NewSharedCatalogServiceis indeedmargins port.TenantMarginRepo, andnilis passed inmain.goline 123. However, this field is not referenced anywhere inSharedCatalogServicemethods, so there is no nil-deref risk at present. This suggests either incomplete wiring (the field should be used but isn't) or incomplete implementation (the field was added but usage wasn't finished). Clarify the intent: either remove the unusedmarginsparameter from the constructor, or add the margin-aware enrichment logic that uses it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/api/main.go` around lines 116 - 124, The constructor call to NewSharedCatalogService passes nil for the 4th parameter (margins port.TenantMarginRepo) but that dependency is unused; either remove the margins parameter from the SharedCatalogService constructor/signature and any related fields (to simplify wiring), or implement margin-aware enrichment logic that consumes the TenantMarginRepo: update NewSharedCatalogService, the SharedCatalogService struct and its methods to accept and call the margins repo where pricing/enrichment occurs (identify functions using sharedCatalogSvc, NewSharedCatalogService, and the TenantMarginRepo type to locate the change), and replace the nil in main.go with the actual repo (postgres.NewTenantMarginRepo(pool)) if implementing; ensure tests/usage are updated accordingly.internal/api/handler/assessment_handler.go-34-34 (1)
34-34:⚠️ Potential issue | 🟡 MinorAvoid leaking internal error details to clients.
Including
err.Error()in the response may expose sensitive implementation details (database errors, stack traces, etc.). Return a generic message and log the detailed error server-side.🛡️ Proposed fix
+ slog.Error("assessment: failed to start", "tenant_id", ac.TenantID, "error", err) - response.Error(w, http.StatusInternalServerError, "failed to start assessment: "+err.Error()) + response.Error(w, http.StatusInternalServerError, "failed to start assessment")Note: You'll need to import
log/slog.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/api/handler/assessment_handler.go` at line 34, Replace the direct exposure of err in the HTTP response: instead of returning "failed to start assessment: "+err.Error(), log the detailed error server-side (import log/slog and call slog.Error with context such as "failed to start assessment" and the err value) and send a generic client-facing message via response.Error (e.g., "failed to start assessment") without including err.Error(); update the code around the existing response.Error call and ensure the detailed error is recorded with slog.Error before writing the generic response.internal/domain/strategy.go-66-74 (1)
66-74:⚠️ Potential issue | 🟡 MinorClamp progress at the lower bound too.
If
CurrentProgressgoes negative, this returns a negative percentage even though the contract says0-100. Clamp values below zero before returning.Suggested fix
func (g *StrategyGoal) ProgressPct() float64 { if g.TargetAmount <= 0 { return 0 } pct := (g.CurrentProgress / g.TargetAmount) * 100 + if pct < 0 { + return 0 + } if pct > 100 { return 100 } return pct }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/domain/strategy.go` around lines 66 - 74, In StrategyGoal.ProgressPct(), ensure the returned percentage is clamped to the 0–100 range: after handling TargetAmount <= 0, compute pct := (g.CurrentProgress / g.TargetAmount) * 100 and then clamp pct to be at least 0 and at most 100 (e.g., use math.Max(0, pct) and math.Min(100, pct) or equivalent) before returning; this prevents negative percentages when CurrentProgress is negative while preserving the existing upper-bound logic.internal/adapter/postgres/strategy_version_repo.go-37-54 (1)
37-54:⚠️ Potential issue | 🟡 MinorHandle JSON unmarshal errors to avoid returning corrupt data.
If
json.Unmarshalfails, the method returns aStrategyVersionwith zero-valueGoalsandSearchParams, which could cause downstream logic errors.🛡️ Proposed fix
- json.Unmarshal(goalsJSON, &sv.Goals) - json.Unmarshal(paramsJSON, &sv.SearchParams) + if err := json.Unmarshal(goalsJSON, &sv.Goals); err != nil { + return nil, fmt.Errorf("unmarshal goals: %w", err) + } + if err := json.Unmarshal(paramsJSON, &sv.SearchParams); err != nil { + return nil, fmt.Errorf("unmarshal search params: %w", err) + } return &sv, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/postgres/strategy_version_repo.go` around lines 37 - 54, In StrategyVersionRepo.GetByID, handle errors from json.Unmarshal for goalsJSON and paramsJSON instead of ignoring them: after scanning into goalsJSON and paramsJSON, call json.Unmarshal for each into &sv.Goals and &sv.SearchParams, check the returned error, and if non-nil return nil and a wrapped error (e.g., fmt.Errorf("unmarshal goals: %w", err) or "unmarshal search params"), so you don't return a StrategyVersion with corrupt/zero-value Goals or SearchParams; reference the GetByID method and the goalsJSON/paramsJSON and sv.Goals/sv.SearchParams variables when making the change.internal/adapter/postgres/strategy_version_repo.go-21-35 (1)
21-35:⚠️ Potential issue | 🟡 MinorHandle JSON marshal errors to prevent silent data corruption.
json.Marshalcan fail in edge cases. While unlikely with these domain types, ignoring errors could lead to empty JSONB columns being written.🛡️ Proposed fix
func (r *StrategyVersionRepo) Create(ctx context.Context, sv *domain.StrategyVersion) error { - goalsJSON, _ := json.Marshal(sv.Goals) - paramsJSON, _ := json.Marshal(sv.SearchParams) + goalsJSON, err := json.Marshal(sv.Goals) + if err != nil { + return fmt.Errorf("marshal goals: %w", err) + } + paramsJSON, err := json.Marshal(sv.SearchParams) + if err != nil { + return fmt.Errorf("marshal search params: %w", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/postgres/strategy_version_repo.go` around lines 21 - 35, In StrategyVersionRepo.Create, don’t ignore errors from json.Marshal for sv.Goals and sv.SearchParams (the goalsJSON and paramsJSON calls); capture and check both marshal errors and return a wrapped error (e.g., fmt.Errorf("marshal goals/search_params: %w")) before executing the INSERT so you never write empty/invalid JSONB to the database and provide clear context for failures.internal/service/assessment_service.go-183-191 (1)
183-191:⚠️ Potential issue | 🟡 MinorPartial persistence: probe/category save failures only logged.
If
SaveProbeResultsorSaveCategoryEligibilitiesfails, the fingerprint is still created but detailed data is lost. This could cause inconsistent state where the fingerprint exists but drill-down data doesn't.Consider whether these should be transactional or at minimum return warnings to the caller.
🤖 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 183 - 191, The current flow creates the fingerprint with s.fingerprints.Create(ctx, fp) but only logs failures from SaveProbeResults and SaveCategoryEligibilities, leaving partial state; update the logic around Create, SaveProbeResults and SaveCategoryEligibilities so failures are not silently swallowed: either wrap these operations in a transaction if the repository supports it, or if not, on any SaveProbeResults(ctx, fingerprintID, tenantID, ...) or SaveCategoryEligibilities(ctx, fingerprintID, tenantID, ...) error call s.fingerprints.Delete(ctx, fingerprintID) (or another compensating action) and return the error to the caller instead of only logging, ensuring fingerprintID/tenantID are used to locate and revert the created fp.internal/adapter/postgres/credit_repo.go-44-52 (1)
44-52:⚠️ Potential issue | 🟡 Minor
Debitdoesn't verify affected rows — silent failure if account missing.If no account exists for the tenant, the UPDATE affects 0 rows and returns no error. The caller has no way to know the debit didn't actually happen.
🛡️ Proposed fix: check affected rows
func (r *CreditAccountRepo) Debit(ctx context.Context, tenantID domain.TenantID, amount int) error { - _, err := r.pool.Exec(ctx, ` + result, err := r.pool.Exec(ctx, ` UPDATE credit_accounts SET used_this_month = used_this_month + $2 WHERE tenant_id = $1 `, tenantID, amount) if err != nil { return fmt.Errorf("debit credits: %w", err) } + if result.RowsAffected() == 0 { + return fmt.Errorf("debit credits: account not found for tenant %s", tenantID) + } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/postgres/credit_repo.go` around lines 44 - 52, The Debit method on CreditAccountRepo uses r.pool.Exec but doesn't check the number of affected rows, so an UPDATE that touches 0 rows (missing account) silently succeeds; change Debit to capture the Exec result (cmdTag, err := r.pool.Exec(...)), check cmdTag.RowsAffected() and, if zero, return a clear error (e.g. fmt.Errorf("debit credits: no account for tenant %v", tenantID) or a domain-specific NotFound error) instead of nil; keep existing error wrapping for Exec failures.internal/service/shared_catalog_service.go-159-165 (1)
159-165:⚠️ Potential issue | 🟡 MinorErrors from
eligibility.Setandbrands.UpdateGatingsilently ignored.Similar to the enrichment flow, these persistence operations fail silently.
🛡️ Proposed fix
- s.eligibility.Set(ctx, result) + if err := s.eligibility.Set(ctx, result); err != nil { + slog.Warn("shared-catalog: failed to save eligibility", "asin", asin, "error", err) + } // Update brand gating info in shared catalog product, _ := s.catalog.GetByASIN(ctx, asin) if product != nil && product.Brand != "" && !eligible { - s.brands.UpdateGating(ctx, strings.ToLower(strings.TrimSpace(product.Brand)), "brand_gated") + if err := s.brands.UpdateGating(ctx, strings.ToLower(strings.TrimSpace(product.Brand)), "brand_gated"); err != nil { + slog.Warn("shared-catalog: failed to update brand gating", "brand", product.Brand, "error", err) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/shared_catalog_service.go` around lines 159 - 165, The calls to eligibility.Set and brands.UpdateGating (and the ignored error from s.catalog.GetByASIN) currently drop errors silently; update the block so you capture and handle their returned errors: check the error from s.catalog.GetByASIN (instead of using the blank identifier), then call s.eligibility.Set(ctx, result) and handle its error (log and return/wrap as appropriate for this method), and likewise call s.brands.UpdateGating(...) and handle its error instead of ignoring it; ensure you include context in log messages and return a wrapped error from the surrounding function when persistence updates fail.internal/service/shared_catalog_service.go-104-105 (1)
104-105:⚠️ Potential issue | 🟡 MinorErrors from
UpsertProductandIncrementEnrichmentsilently ignored.If the catalog update fails after spending credits and calling SP-API, the enriched data is lost. Consider at minimum logging these errors.
🛡️ Proposed fix
- s.catalog.UpsertProduct(ctx, product) - s.catalog.IncrementEnrichment(ctx, asin) + if err := s.catalog.UpsertProduct(ctx, product); err != nil { + slog.Warn("shared-catalog: failed to upsert product", "asin", asin, "error", err) + } + if err := s.catalog.IncrementEnrichment(ctx, asin); err != nil { + slog.Warn("shared-catalog: failed to increment enrichment", "asin", asin, "error", err) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/shared_catalog_service.go` around lines 104 - 105, The calls to s.catalog.UpsertProduct(ctx, product) and s.catalog.IncrementEnrichment(ctx, asin) ignore returned errors; change them to capture their error returns, log any non-nil error (including context like asin and product identifiers) using the service logger (e.g., s.logger or existing logging helper) and propagate or handle the error appropriately (return it from the enclosing method or record a metric) so failures aren’t silently dropped; update the caller flow to handle the propagated error if you choose to return it.internal/adapter/postgres/suggestion_repo.go-123-135 (1)
123-135:⚠️ Potential issue | 🟡 MinorMissing
rows.Err()check after iteration.After iterating with
rows.Next(), you should checkrows.Err()to catch any errors that occurred during iteration (e.g., network issues mid-stream).🛡️ Proposed fix
func (r *SuggestionRepo) scanRows(rows interface{ Next() bool; Scan(...any) error }) ([]domain.DiscoverySuggestion, error) { var suggestions []domain.DiscoverySuggestion for rows.Next() { var s domain.DiscoverySuggestion if err := rows.Scan(&s.ID, &s.TenantID, &s.StrategyVersionID, &s.GoalID, &s.ASIN, &s.Title, &s.Brand, &s.Category, &s.BuyBoxPrice, &s.EstimatedMargin, &s.BSRRank, &s.SellerCount, &s.Reason, &s.Status, &s.DealID, &s.CreatedAt, &s.ResolvedAt); err != nil { return nil, err } suggestions = append(suggestions, s) } + if err, ok := rows.(interface{ Err() error }); ok && err.Err() != nil { + return nil, err.Err() + } return suggestions, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/postgres/suggestion_repo.go` around lines 123 - 135, The scanRows function is missing a rows.Err() check after iterating; update the rows parameter interface in SuggestionRepo.scanRows to include Err() error (e.g., interface{ Next() bool; Scan(...any) error; Err() error }) and after the for rows.Next() loop add a check like if err := rows.Err(); err != nil { return nil, err } so any iteration errors are propagated from scanRows.
Security fixes: 1. suggestion_repo Accept/Dismiss — added tenant_id scoping to prevent cross-tenant modification (UPDATE WHERE tenant_id = $2) 2. suggestion_handler Dismiss — added auth context for tenant scoping 3. discovery_queue_service Accept/Dismiss — pass tenantID through to repo Bug fixes: 4. tenant_eligibility_repo ListEligible — fixed placeholder ordering ($2 was category, $3 was limit — now correctly sequenced) 5. discovery_queue_service — Brand field was assigned from Category; fixed to use BrandID from DiscoveredProduct Race condition fix: 6. credit_repo Debit — atomic SQL check: UPDATE only if (monthly_limit - used_this_month) >= amount. Prevents concurrent requests from both passing HasCredits and over-debiting. Test updates: mock signatures updated for new tenant-scoped interfaces. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
internal/adapter/postgres/credit_repo.go (1)
103-109: Cap maximumlimitto avoid oversized tenant queries.Defaulting non-positive limits is good, but very large positive values can still cause unnecessary DB and memory pressure. Clamp to a sane upper bound.
Suggested guardrail
if limit <= 0 { limit = 50 } + if limit > 500 { + limit = 500 + } rows, err := r.pool.Query(ctx, `🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/postgres/credit_repo.go` around lines 103 - 109, The limit passed to r.pool.Query (variable limit) is only defaulted when <=0 but not capped, so clamp limit to a sane maximum (e.g., MAX_LIMIT = 1000) before calling r.pool.Query to prevent huge queries; update the logic around the existing check (the block that currently sets limit = 50) to enforce: if limit <= 0 set default (50), else if limit > MAX_LIMIT set limit = MAX_LIMIT, then call r.pool.Query with tenantID and the clamped limit.internal/service/discovery_queue_service_test.go (3)
111-127: MockAcceptignorestenantIDin lookup — reduces test fidelity.The mock receives
tenantIDbut only filters byidin the loop. This means tests won't catch bugs where the service incorrectly omits tenant scoping. Consider filtering by bothIDandTenantIDto mirror production behavior.♻️ Proposed fix to add tenant filtering
func (r *mockSuggestionRepo) Accept(_ context.Context, tenantID domain.TenantID, id domain.SuggestionID, dealID domain.DealID) error { r.mu.Lock() defer r.mu.Unlock() if r.acceptErr != nil { return r.acceptErr } for i := range r.suggestions { - if r.suggestions[i].ID == id { + if r.suggestions[i].ID == id && r.suggestions[i].TenantID == tenantID { r.suggestions[i].Status = domain.SuggestionStatusAccepted r.suggestions[i].DealID = &dealID now := time.Now() r.suggestions[i].ResolvedAt = &now return nil } } return fmt.Errorf("suggestion %s not found", id) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/discovery_queue_service_test.go` around lines 111 - 127, The mockSuggestionRepo.Accept currently ignores the provided tenantID when locating a suggestion, weakening tests; update the Accept method to match on both id and tenantID (e.g., check r.suggestions[i].ID == id && r.suggestions[i].TenantID == tenantID) before marking Status, DealID, and ResolvedAt, and return a not-found error when no matching suggestion for that tenant exists so the mock mirrors real tenant-scoped lookup behavior.
129-144: MockDismissalso ignorestenantID— same concern asAccept.Apply the same tenant filtering fix for consistency and better test fidelity.
♻️ Proposed fix
func (r *mockSuggestionRepo) Dismiss(_ context.Context, tenantID domain.TenantID, id domain.SuggestionID) error { r.mu.Lock() defer r.mu.Unlock() if r.dismissErr != nil { return r.dismissErr } for i := range r.suggestions { - if r.suggestions[i].ID == id { + if r.suggestions[i].ID == id && r.suggestions[i].TenantID == tenantID { r.suggestions[i].Status = domain.SuggestionStatusDismissed now := time.Now() r.suggestions[i].ResolvedAt = &now return nil } } return fmt.Errorf("suggestion %s not found", id) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/discovery_queue_service_test.go` around lines 129 - 144, The mockSuggestionRepo.Dismiss method currently ignores the tenantID parameter similar to mockSuggestionRepo.Accept; update Dismiss to check that the suggestion's TenantID equals the passed tenantID before modifying Status/ResolvedAt so it only dismisses suggestions for the correct tenant (mirror the tenant filtering logic used in Accept). Ensure you lock/unlock the mutex as before, return the same not-found error if no matching suggestion with both ID and TenantID is found, and still honor r.dismissErr early-return behavior.
903-914: Preferstrings.Containsover custom helpers.Go's standard library provides
strings.Containswhich is more idiomatic and well-tested.♻️ Simplify using stdlib
+import "strings" + // In test file, replace usage: -if !containsSubstring(s.Reason, expected) { +if !strings.Contains(s.Reason, expected) { -// Remove these helper functions: -func containsSubstring(s, substr string) bool { - return len(s) >= len(substr) && (s == substr || len(s) > 0 && containsCheck(s, substr)) -} - -func containsCheck(s, substr string) bool { - for i := 0; i <= len(s)-len(substr); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return false -}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/discovery_queue_service_test.go` around lines 903 - 914, Replace the custom substring helpers with the stdlib: remove containsSubstring and containsCheck and use strings.Contains wherever those functions are used; update imports to include "strings" and replace calls to containsSubstring(s, substr) with strings.Contains(s, substr) to keep behavior identical and rely on the well-tested standard implementation.internal/service/discovery_queue_service.go (1)
44-52: Consider whether "fail open" on CountToday error is intentional.When
CountTodayfails, the error is logged buttodayCountremains 0, allowing the full daily cap of suggestions. If the DB is temporarily unreachable, this could result in exceeding the intended daily limit across retries.If this is intentional resilience (prefer to generate suggestions rather than block), a brief comment would clarify the design choice. Otherwise, consider returning early on error.
🛡️ Option: Fail closed if count retrieval is critical
todayCount, err := s.suggestions.CountToday(ctx, tenantID) if err != nil { - slog.Warn("discovery-queue: failed to count today's suggestions", "error", err) + slog.Warn("discovery-queue: failed to count today's suggestions", "error", err) + return nil, fmt.Errorf("count today's suggestions: %w", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/discovery_queue_service.go` around lines 44 - 52, The current code treats a CountToday error as "fail open" (logs and treats todayCount as 0) which may allow exceeding MaxDailySuggestions; update the handler for s.suggestions.CountToday(ctx, tenantID) to fail closed by returning the error (propagate it up) instead of continuing, or if the fail-open behavior is intentional, add a clear comment above the call explaining the resilience rationale; refer to s.suggestions.CountToday, todayCount, and MaxDailySuggestions when making the change so the check either aborts on error or documents the deliberate choice.
🤖 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/postgres/credit_repo.go`:
- Around line 44-55: In CreditAccountRepo.Debit ensure you reject non-positive
amounts before running the UPDATE: validate the amount parameter (in the Debit
method) and return an error if amount <= 0; this prevents accidental credit
minting or no-op/negative effects and keeps the existing atomic DB update and
error handling (result.RowsAffected() check) intact.
- Around line 102-124: In ListByTenant, after iterating rows in the for
rows.Next() loop, add a check for rows.Err() and return that error if non-nil to
surface any iteration/driver errors; specifically update the function
CreditTransactionRepo.ListByTenant to inspect rows.Err() (using the variable
rows) before returning txs so partial reads don't get silently ignored.
---
Nitpick comments:
In `@internal/adapter/postgres/credit_repo.go`:
- Around line 103-109: The limit passed to r.pool.Query (variable limit) is only
defaulted when <=0 but not capped, so clamp limit to a sane maximum (e.g.,
MAX_LIMIT = 1000) before calling r.pool.Query to prevent huge queries; update
the logic around the existing check (the block that currently sets limit = 50)
to enforce: if limit <= 0 set default (50), else if limit > MAX_LIMIT set limit
= MAX_LIMIT, then call r.pool.Query with tenantID and the clamped limit.
In `@internal/service/discovery_queue_service_test.go`:
- Around line 111-127: The mockSuggestionRepo.Accept currently ignores the
provided tenantID when locating a suggestion, weakening tests; update the Accept
method to match on both id and tenantID (e.g., check r.suggestions[i].ID == id
&& r.suggestions[i].TenantID == tenantID) before marking Status, DealID, and
ResolvedAt, and return a not-found error when no matching suggestion for that
tenant exists so the mock mirrors real tenant-scoped lookup behavior.
- Around line 129-144: The mockSuggestionRepo.Dismiss method currently ignores
the tenantID parameter similar to mockSuggestionRepo.Accept; update Dismiss to
check that the suggestion's TenantID equals the passed tenantID before modifying
Status/ResolvedAt so it only dismisses suggestions for the correct tenant
(mirror the tenant filtering logic used in Accept). Ensure you lock/unlock the
mutex as before, return the same not-found error if no matching suggestion with
both ID and TenantID is found, and still honor r.dismissErr early-return
behavior.
- Around line 903-914: Replace the custom substring helpers with the stdlib:
remove containsSubstring and containsCheck and use strings.Contains wherever
those functions are used; update imports to include "strings" and replace calls
to containsSubstring(s, substr) with strings.Contains(s, substr) to keep
behavior identical and rely on the well-tested standard implementation.
In `@internal/service/discovery_queue_service.go`:
- Around line 44-52: The current code treats a CountToday error as "fail open"
(logs and treats todayCount as 0) which may allow exceeding MaxDailySuggestions;
update the handler for s.suggestions.CountToday(ctx, tenantID) to fail closed by
returning the error (propagate it up) instead of continuing, or if the fail-open
behavior is intentional, add a clear comment above the call explaining the
resilience rationale; refer to s.suggestions.CountToday, todayCount, and
MaxDailySuggestions when making the change so the check either aborts on error
or documents the deliberate choice.
🪄 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: 8e3be855-85bd-4d95-8a7c-74a9a11cdd39
📒 Files selected for processing (7)
internal/adapter/postgres/credit_repo.gointernal/adapter/postgres/suggestion_repo.gointernal/adapter/postgres/tenant_eligibility_repo.gointernal/api/handler/suggestion_handler.gointernal/port/suggestion.gointernal/service/discovery_queue_service.gointernal/service/discovery_queue_service_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/port/suggestion.go
🚧 Files skipped from review as they are similar to previous changes (3)
- internal/api/handler/suggestion_handler.go
- internal/adapter/postgres/tenant_eligibility_repo.go
- internal/adapter/postgres/suggestion_repo.go
Comment on lines
+102
to
+124
| func (r *CreditTransactionRepo) ListByTenant(ctx context.Context, tenantID domain.TenantID, limit int) ([]domain.CreditTransaction, error) { | ||
| if limit <= 0 { | ||
| limit = 50 | ||
| } | ||
| rows, err := r.pool.Query(ctx, ` | ||
| SELECT id, tenant_id, amount, action, reference, created_at | ||
| FROM credit_transactions WHERE tenant_id = $1 ORDER BY created_at DESC LIMIT $2 | ||
| `, tenantID, limit) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| defer rows.Close() | ||
|
|
||
| var txs []domain.CreditTransaction | ||
| for rows.Next() { | ||
| var tx domain.CreditTransaction | ||
| if err := rows.Scan(&tx.ID, &tx.TenantID, &tx.Amount, &tx.Action, &tx.Reference, &tx.CreatedAt); err != nil { | ||
| return nil, err | ||
| } | ||
| txs = append(txs, tx) | ||
| } | ||
| return txs, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify this function currently lacks post-iteration error handling.
rg -nP --type go -C3 '\bfunc \(r \*CreditTransactionRepo\) ListByTenant\b|for rows\.Next\(\)|rows\.Err\(' internal/adapter/postgres/credit_repo.goRepository: alastrat/amazonagent
Length of output: 551
🏁 Script executed:
# View the complete ListByTenant function
sed -n '102,124p' internal/adapter/postgres/credit_repo.goRepository: alastrat/amazonagent
Length of output: 765
🏁 Script executed:
# Check if there are other functions with similar row iteration patterns to see consistency
rg -n 'for rows\.Next\(\)' --type go -A10 internal/adapter/postgres/ | head -50Repository: alastrat/amazonagent
Length of output: 4098
Add rows.Err() check after row iteration to prevent silent partial reads.
The loop terminates when rows.Next() returns false, but any driver/cursor error during iteration can be masked without an explicit error check. Add rows.Err() before returning to surface such errors.
Proposed fix
func (r *CreditTransactionRepo) ListByTenant(ctx context.Context, tenantID domain.TenantID, limit int) ([]domain.CreditTransaction, error) {
if limit <= 0 {
limit = 50
}
rows, err := r.pool.Query(ctx, `
SELECT id, tenant_id, amount, action, reference, created_at
FROM credit_transactions WHERE tenant_id = $1 ORDER BY created_at DESC LIMIT $2
`, tenantID, limit)
if err != nil {
return nil, err
}
defer rows.Close()
var txs []domain.CreditTransaction
for rows.Next() {
var tx domain.CreditTransaction
if err := rows.Scan(&tx.ID, &tx.TenantID, &tx.Amount, &tx.Action, &tx.Reference, &tx.CreatedAt); err != nil {
return nil, err
}
txs = append(txs, tx)
}
+ if err := rows.Err(); err != nil {
+ return nil, err
+ }
return txs, 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
| func (r *CreditTransactionRepo) ListByTenant(ctx context.Context, tenantID domain.TenantID, limit int) ([]domain.CreditTransaction, error) { | |
| if limit <= 0 { | |
| limit = 50 | |
| } | |
| rows, err := r.pool.Query(ctx, ` | |
| SELECT id, tenant_id, amount, action, reference, created_at | |
| FROM credit_transactions WHERE tenant_id = $1 ORDER BY created_at DESC LIMIT $2 | |
| `, tenantID, limit) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| var txs []domain.CreditTransaction | |
| for rows.Next() { | |
| var tx domain.CreditTransaction | |
| if err := rows.Scan(&tx.ID, &tx.TenantID, &tx.Amount, &tx.Action, &tx.Reference, &tx.CreatedAt); err != nil { | |
| return nil, err | |
| } | |
| txs = append(txs, tx) | |
| } | |
| return txs, nil | |
| } | |
| func (r *CreditTransactionRepo) ListByTenant(ctx context.Context, tenantID domain.TenantID, limit int) ([]domain.CreditTransaction, error) { | |
| if limit <= 0 { | |
| limit = 50 | |
| } | |
| rows, err := r.pool.Query(ctx, ` | |
| SELECT id, tenant_id, amount, action, reference, created_at | |
| FROM credit_transactions WHERE tenant_id = $1 ORDER BY created_at DESC LIMIT $2 | |
| `, tenantID, limit) | |
| if err != nil { | |
| return nil, err | |
| } | |
| defer rows.Close() | |
| var txs []domain.CreditTransaction | |
| for rows.Next() { | |
| var tx domain.CreditTransaction | |
| if err := rows.Scan(&tx.ID, &tx.TenantID, &tx.Amount, &tx.Action, &tx.Reference, &tx.CreatedAt); err != nil { | |
| return nil, err | |
| } | |
| txs = append(txs, tx) | |
| } | |
| if err := rows.Err(); err != nil { | |
| return nil, err | |
| } | |
| return txs, nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/adapter/postgres/credit_repo.go` around lines 102 - 124, In
ListByTenant, after iterating rows in the for rows.Next() loop, add a check for
rows.Err() and return that error if non-nil to surface any iteration/driver
errors; specifically update the function CreditTransactionRepo.ListByTenant to
inspect rows.Err() (using the variable rows) before returning txs so partial
reads don't get silently ignored.
Code reuse: - Extract scanProduct/scanProductRows helpers in SharedCatalogRepo (eliminated 4 copy-pasted scan blocks) - Extract scanVersion helper in StrategyVersionRepo (3 duplicates) - Extract domain.NormalizeBrandName (was inline in 5 places) - Extract domain.EstimateMarginPct + EstimatedWholesaleRatio constant (magic 0.4 was in 3 places) Quality: - Fix leaky abstraction: AssessmentService now calls SharedCatalogService.RecordEligibility() instead of reaching into unexported eligibility field - Use domain constants instead of string literals in SQL repos (SuggestionStatusPending, StrategyStatusActive, etc.) - Fix swallowed scan errors in EligibilityFingerprintRepo.Get Efficiency: - Remove redundant HasCredits pre-check in Spend — atomic Debit SQL already enforces the balance check - Deduplicate brands in RecordFromScan — upsert once per unique brand instead of per product 269 tests still passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (5)
internal/service/credit_service_test.go (1)
519-527: Misleading test name:TestCreditService_Spend_HasCreditsError.The test sets
accts.getErrto simulate a failure, butSpendno longer callsHasCredits(it uses atomicDebitdirectly). The test still works becausegetErris never checked in the mock'sDebitpath—it actually fails because the account isn't seeded. Consider renaming toTestCreditService_Spend_AccountNotFoundor seeding an account and settingdebitErrto test the intended failure mode.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/credit_service_test.go` around lines 519 - 527, The test TestCreditService_Spend_HasCreditsError is misleading because Spend no longer calls HasCredits; update the test to either seed an account and set the mock's debit error (h.accts.debitErr) to simulate a Debit failure, or rename the test to TestCreditService_Spend_AccountNotFound and assert the error when no account is seeded; locate the test via newCreditTestHarness and h.accts references and adjust the setup to exercise the Debit path used by svc.Spend rather than relying on h.accts.getErr.internal/service/shared_catalog_service.go (2)
100-101: Persistence errors silently ignored.Several persistence calls discard errors:
- Line 100-101:
UpsertProductandIncrementEnrichment- Line 155:
eligibility.Set- Line 241-242:
brands.UpsertandIncrementProductCountWhile these are non-critical for the immediate return value, silent failures could lead to stale or missing catalog data. Consider logging errors at minimum.
Also applies to: 155-155, 241-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/shared_catalog_service.go` around lines 100 - 101, Persistence calls are ignoring returned errors which can hide failures; modify calls to UpsertProduct(ctx, product), IncrementEnrichment(ctx, asin), eligibility.Set(...), brands.Upsert(...), and brands.IncrementProductCount(...) to capture their error returns and at minimum log them via the service logger (e.g. s.logger.Errorf or s.logger.Error) with contextual info (asin, product ID, brand ID) so failures are recorded without changing control flow; ensure each ignored call is updated to check err != nil and log the error.
202-204: Double normalization:upsertBrandreceives already-normalized key.The loop at line 202 iterates over
uniqueBrandskeyed by normalized names, but passesnormalizedtoupsertBrand, which normalizes again at line 231. Harmless but redundant.♻️ Minor optimization
// Upsert each brand once. - for normalized, category := range uniqueBrands { - s.upsertBrand(ctx, normalized, category) + for _, p := range products { + if p.Brand != "" { + normalized := domain.NormalizeBrandName(p.Brand) + if cat, ok := uniqueBrands[normalized]; ok { + s.upsertBrand(ctx, p.Brand, cat) // pass original brand name + delete(uniqueBrands, normalized) + } + } }Or simply document that
upsertBrandhandles idempotent normalization.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/service/shared_catalog_service.go` around lines 202 - 204, The loop iterates uniqueBrands keyed by normalized names and then calls upsertBrand(ctx, normalized, category), but upsertBrand also normalizes its input again, causing redundant work; update upsertBrand to assume it receives an already-normalized brand (remove the internal NormalizeBrand call) and add a short comment on upsertBrand stating the parameter must be normalized (or alternatively accept an explicit alreadyNormalized flag), leaving the caller (the loop over uniqueBrands) as-is; reference: the for loop over uniqueBrands and the upsertBrand function.internal/adapter/postgres/brand_catalog_repo.go (1)
68-79:UpdateGatingandIncrementProductCountsilently succeed when no row matches.Both methods return
nileven when zero rows are affected (brand doesn't exist). This may mask issues where callers expect the brand to exist. Consider checkingRowsAffected()if caller needs to know whether the update occurred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/adapter/postgres/brand_catalog_repo.go` around lines 68 - 79, Both UpdateGating and IncrementProductCount currently ignore whether the UPDATE matched any rows and thus return nil even when no brand exists; change each to capture the Exec result (e.g., tag, err := r.pool.Exec(...)), check tag.RowsAffected() and if it equals 0 return a not-found error (either a package-level ErrNotFound or fmt.Errorf("brand not found: %s", normalizedName)) instead of nil; otherwise return err (or nil) as before. Ensure this logic is applied in UpdateGating and IncrementProductCount so callers can detect missing brands.internal/domain/fees.go (1)
9-15: Document the hardcoded assumptions for clarity.The function uses fixed values for
weightLbs = 1.0andisOversized = false. While acceptable for rough estimates, consider adding a brief doc comment noting these assumptions, since FBA fees vary significantly by weight (tests show $3.22 for <1lb vs $4.75+ for heavier items).📝 Suggested documentation enhancement
// EstimateMarginPct returns an estimated net margin percentage for a given // Amazon price, using the default wholesale ratio and standard-size FBA fees. +// Note: Assumes 1.0 lb standard-size product. Actual margins vary with weight/size. func EstimateMarginPct(amazonPrice float64) float64 {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/domain/fees.go` around lines 9 - 15, EstimateMarginPct currently calls CalculateFBAFees using hardcoded weightLbs = 1.0 and isOversized = false, which makes it a rough approximation; add a brief doc comment above the EstimateMarginPct function that states these assumptions (weight fixed at 1.0 lb and not oversized), that FBA fees vary significantly by weight and size, and that this method is intended for quick estimates only (or note that callers should use CalculateFBAFees directly for precise results or that future work could surface weight/oversized as parameters). Reference the CalculateFBAFees call and the EstimatedWholesaleRatio constant in the comment so readers can locate the related 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 `@internal/adapter/postgres/brand_catalog_repo.go`:
- Around line 46-66: In BrandCatalogRepo.ListByCategory, after iterating rows
with rows.Next() you must check for iteration errors by calling rows.Err();
update the function to, once the loop completes and before returning brands,
call if err := rows.Err(); err != nil { return nil, err } so any errors
encountered during scanning/iteration are propagated; reference the
ListByCategory method and the rows variable when making this change.
In `@internal/adapter/postgres/strategy_version_repo.go`:
- Around line 22-23: The code currently ignores json.Marshal errors when
serializing sv.Goals and sv.SearchParams (assignments to goalsJSON and
paramsJSON, also repeated at the later pair) which can mask invalid
StrategyVersion payloads; modify both places to capture the error (e.g.,
goalsJSON, err := json.Marshal(sv.Goals) and paramsJSON, err :=
json.Marshal(sv.SearchParams), and handle err), returning or propagating a
descriptive error (wrap with context mentioning StrategyVersion or sv) from the
surrounding function instead of discarding it so callers get a clear failure
when serialization fails.
- Around line 86-95: List iterates DB rows but doesn't check rows.Err() after
the loop, so any deferred streaming errors can be lost; after the for
rows.Next() loop in the List implementation, call if err := rows.Err(); err !=
nil { return nil, err } (ensuring you reference the existing rows and return
signature), and also ensure rows.Close() is deferred where rows is created if
not already done.
- Around line 108-131: SetStatus currently updates by id only and Activate runs
an unguarded archive Exec without error handling; change SetStatus(ctx, id,
status) to require tenantID (e.g., SetStatus(ctx, tenantID, id, status)) and
update its SQL to include tenant_id in the WHERE clause and pass tenantID as a
parameter for both the simple and timestamped UPDATEs (ensure
activated_at/rolled_back_at use the same now value param). Make Activate perform
both the archive and the new activation inside a single transaction (use
r.pool.BeginTx / tx.Exec / tx.Commit/tx.Rollback), ensure the archive UPDATE
checks tenant_id and its Exec error is checked/propagated, and call the updated
SetStatus within that transaction or inline the update against the same tx to
guarantee atomicity and prevent cross-tenant mutation.
In `@internal/adapter/postgres/suggestion_repo.go`:
- Around line 80-95: Update Accept and Dismiss to guard transitions and error on
no-op: in SuggestionRepo.Accept and SuggestionRepo.Dismiss add "AND status =
'pending'" to their WHERE clauses so they only update pending suggestions,
capture the Exec result (command tag) instead of ignoring it, and if
RowsAffected() == 0 return a meaningful error (e.g., not found/invalid
transition) rather than nil; preserve setting deal_id/resolved_at in Accept and
resolved_at in Dismiss when the update succeeds.
- Around line 125-136: The scanRows function currently iterates rows but doesn't
check for iterator errors; update the parameter type to include Err() error
(e.g., interface{ Next() bool; Scan(...any) error; Err() error }) and after the
for loop call rows.Err() and return any error encountered, ensuring scanRows (in
SuggestionRepo) returns that error instead of silently succeeding when iteration
fails.
In `@internal/service/credit_service.go`:
- Around line 36-41: The re-fetch after ResetMonthly currently discards the
error from s.accounts.Get and may return a stale account; update the code in the
block that checks account.ResetAt so that the second call to s.accounts.Get(ctx,
tenantID) assigns both account and err, checks err, and handles it (e.g., return
the error up or log and return) instead of ignoring it; ensure this change
touches the variables used (account, err) alongside functions account.ResetAt,
s.accounts.ResetMonthly and s.accounts.Get so UsedThisMonth is read from a
fresh, validated account.
In `@internal/service/shared_catalog_service.go`:
- Around line 120-133: In CheckEligibility, you're spending credits via
s.credits.SpendIfAvailable before verifying s.spapi; move the s.spapi nil check
to occur before any SpendIfAvailable call (same pattern as EnrichProduct): if
s.spapi == nil return the default eligible TenantEligibility immediately without
touching s.credits, and only attempt to call SpendIfAvailable when s.spapi is
non-nil; update the logic around s.credits.SpendIfAvailable, tenantID and asin
accordingly so credits are consumed only when SP-API will actually be invoked.
- Around line 56-71: The code currently calls s.credits.SpendIfAvailable(...)
before verifying s.spapi, causing a credit to be consumed even if SP-API is
unavailable; move the s.spapi nil-check above the SpendIfAvailable call (or only
call SpendIfAvailable after confirming s.spapi != nil) so credits are only spent
when an actual SP-API enrichment attempt will occur; update logic around
s.credits, SpendIfAvailable, s.spapi, cached, tenantID and asin accordingly so
stale return behavior remains unchanged when s.spapi is nil.
---
Nitpick comments:
In `@internal/adapter/postgres/brand_catalog_repo.go`:
- Around line 68-79: Both UpdateGating and IncrementProductCount currently
ignore whether the UPDATE matched any rows and thus return nil even when no
brand exists; change each to capture the Exec result (e.g., tag, err :=
r.pool.Exec(...)), check tag.RowsAffected() and if it equals 0 return a
not-found error (either a package-level ErrNotFound or fmt.Errorf("brand not
found: %s", normalizedName)) instead of nil; otherwise return err (or nil) as
before. Ensure this logic is applied in UpdateGating and IncrementProductCount
so callers can detect missing brands.
In `@internal/domain/fees.go`:
- Around line 9-15: EstimateMarginPct currently calls CalculateFBAFees using
hardcoded weightLbs = 1.0 and isOversized = false, which makes it a rough
approximation; add a brief doc comment above the EstimateMarginPct function that
states these assumptions (weight fixed at 1.0 lb and not oversized), that FBA
fees vary significantly by weight and size, and that this method is intended for
quick estimates only (or note that callers should use CalculateFBAFees directly
for precise results or that future work could surface weight/oversized as
parameters). Reference the CalculateFBAFees call and the EstimatedWholesaleRatio
constant in the comment so readers can locate the related logic.
In `@internal/service/credit_service_test.go`:
- Around line 519-527: The test TestCreditService_Spend_HasCreditsError is
misleading because Spend no longer calls HasCredits; update the test to either
seed an account and set the mock's debit error (h.accts.debitErr) to simulate a
Debit failure, or rename the test to TestCreditService_Spend_AccountNotFound and
assert the error when no account is seeded; locate the test via
newCreditTestHarness and h.accts references and adjust the setup to exercise the
Debit path used by svc.Spend rather than relying on h.accts.getErr.
In `@internal/service/shared_catalog_service.go`:
- Around line 100-101: Persistence calls are ignoring returned errors which can
hide failures; modify calls to UpsertProduct(ctx, product),
IncrementEnrichment(ctx, asin), eligibility.Set(...), brands.Upsert(...), and
brands.IncrementProductCount(...) to capture their error returns and at minimum
log them via the service logger (e.g. s.logger.Errorf or s.logger.Error) with
contextual info (asin, product ID, brand ID) so failures are recorded without
changing control flow; ensure each ignored call is updated to check err != nil
and log the error.
- Around line 202-204: The loop iterates uniqueBrands keyed by normalized names
and then calls upsertBrand(ctx, normalized, category), but upsertBrand also
normalizes its input again, causing redundant work; update upsertBrand to assume
it receives an already-normalized brand (remove the internal NormalizeBrand
call) and add a short comment on upsertBrand stating the parameter must be
normalized (or alternatively accept an explicit alreadyNormalized flag), leaving
the caller (the loop over uniqueBrands) as-is; reference: the for loop over
uniqueBrands and the upsertBrand function.
🪄 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: 57acc82d-3c23-477a-9a36-5f63af61c457
📒 Files selected for processing (12)
internal/adapter/postgres/brand_catalog_repo.gointernal/adapter/postgres/brand_repo.gointernal/adapter/postgres/eligibility_fingerprint_repo.gointernal/adapter/postgres/shared_catalog_repo.gointernal/adapter/postgres/strategy_version_repo.gointernal/adapter/postgres/suggestion_repo.gointernal/domain/fees.gointernal/domain/shared_catalog.gointernal/service/assessment_service.gointernal/service/credit_service.gointernal/service/credit_service_test.gointernal/service/shared_catalog_service.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/domain/shared_catalog.go
- internal/adapter/postgres/eligibility_fingerprint_repo.go
- internal/adapter/postgres/shared_catalog_repo.go
- internal/service/assessment_service.go
Comment on lines
+108
to
+131
| func (r *StrategyVersionRepo) SetStatus(ctx context.Context, id domain.StrategyVersionID, status domain.StrategyStatus) error { | ||
| now := time.Now() | ||
| var extra string | ||
| switch status { | ||
| case domain.StrategyStatusActive: | ||
| extra = ", activated_at = $3" | ||
| case domain.StrategyStatusRolledBack: | ||
| extra = ", rolled_back_at = $3" | ||
| default: | ||
| _, err := r.pool.Exec(ctx, `UPDATE strategy_versions SET status = $2 WHERE id = $1`, id, status) | ||
| return err | ||
| } | ||
| _, err := r.pool.Exec(ctx, fmt.Sprintf(`UPDATE strategy_versions SET status = $2%s WHERE id = $1`, extra), id, status, now) | ||
| return err | ||
| } | ||
|
|
||
| func (r *StrategyVersionRepo) Activate(ctx context.Context, tenantID domain.TenantID, id domain.StrategyVersionID) error { | ||
| // Archive current active | ||
| r.pool.Exec(ctx, ` | ||
| UPDATE strategy_versions SET status = $2 WHERE tenant_id = $1 AND status = $3 | ||
| `, tenantID, string(domain.StrategyStatusArchived), string(domain.StrategyStatusActive)) | ||
| // Activate new | ||
| return r.SetStatus(ctx, id, domain.StrategyStatusActive) | ||
| } |
There was a problem hiding this comment.
Enforce tenant-scoped status updates and make activation atomic.
Line 117/Line 120 update by id only, and Line 130 calls that from Activate; this can mutate another tenant’s row if an ID is provided. Also, Line 126 ignores Exec errors and the archive+activate sequence is non-transactional, so partial failure can leave inconsistent active state.
🔒 Suggested fix
-func (r *StrategyVersionRepo) SetStatus(ctx context.Context, id domain.StrategyVersionID, status domain.StrategyStatus) error {
+func (r *StrategyVersionRepo) SetStatus(ctx context.Context, tenantID domain.TenantID, id domain.StrategyVersionID, status domain.StrategyStatus) error {
now := time.Now()
var extra string
switch status {
case domain.StrategyStatusActive:
extra = ", activated_at = $3"
case domain.StrategyStatusRolledBack:
extra = ", rolled_back_at = $3"
default:
- _, err := r.pool.Exec(ctx, `UPDATE strategy_versions SET status = $2 WHERE id = $1`, id, status)
+ _, err := r.pool.Exec(ctx, `UPDATE strategy_versions SET status = $3 WHERE id = $1 AND tenant_id = $2`, id, tenantID, status)
return err
}
- _, err := r.pool.Exec(ctx, fmt.Sprintf(`UPDATE strategy_versions SET status = $2%s WHERE id = $1`, extra), id, status, now)
+ _, err := r.pool.Exec(ctx, fmt.Sprintf(`UPDATE strategy_versions SET status = $3%s WHERE id = $1 AND tenant_id = $2`, extra), id, tenantID, status, now)
return err
}
func (r *StrategyVersionRepo) Activate(ctx context.Context, tenantID domain.TenantID, id domain.StrategyVersionID) error {
- // Archive current active
- r.pool.Exec(ctx, `
- UPDATE strategy_versions SET status = $2 WHERE tenant_id = $1 AND status = $3
- `, tenantID, string(domain.StrategyStatusArchived), string(domain.StrategyStatusActive))
- // Activate new
- return r.SetStatus(ctx, id, domain.StrategyStatusActive)
+ tx, err := r.pool.Begin(ctx)
+ if err != nil {
+ return fmt.Errorf("begin tx: %w", err)
+ }
+ defer tx.Rollback(ctx)
+
+ if _, err := tx.Exec(ctx, `
+ UPDATE strategy_versions
+ SET status = $2
+ WHERE tenant_id = $1 AND status = $3
+ `, tenantID, string(domain.StrategyStatusArchived), string(domain.StrategyStatusActive)); err != nil {
+ return fmt.Errorf("archive current active: %w", err)
+ }
+
+ if _, err := tx.Exec(ctx, `
+ UPDATE strategy_versions
+ SET status = $3, activated_at = $4
+ WHERE id = $1 AND tenant_id = $2
+ `, id, tenantID, string(domain.StrategyStatusActive), time.Now()); err != nil {
+ return fmt.Errorf("activate target version: %w", err)
+ }
+
+ return tx.Commit(ctx)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/adapter/postgres/strategy_version_repo.go` around lines 108 - 131,
SetStatus currently updates by id only and Activate runs an unguarded archive
Exec without error handling; change SetStatus(ctx, id, status) to require
tenantID (e.g., SetStatus(ctx, tenantID, id, status)) and update its SQL to
include tenant_id in the WHERE clause and pass tenantID as a parameter for both
the simple and timestamped UPDATEs (ensure activated_at/rolled_back_at use the
same now value param). Make Activate perform both the archive and the new
activation inside a single transaction (use r.pool.BeginTx / tx.Exec /
tx.Commit/tx.Rollback), ensure the archive UPDATE checks tenant_id and its Exec
error is checked/propagated, and call the updated SetStatus within that
transaction or inline the update against the same tx to guarantee atomicity and
prevent cross-tenant mutation.
Comment on lines
+125
to
+136
| func (r *SuggestionRepo) scanRows(rows interface{ Next() bool; Scan(...any) error }) ([]domain.DiscoverySuggestion, error) { | ||
| var suggestions []domain.DiscoverySuggestion | ||
| for rows.Next() { | ||
| var s domain.DiscoverySuggestion | ||
| if err := rows.Scan(&s.ID, &s.TenantID, &s.StrategyVersionID, &s.GoalID, &s.ASIN, &s.Title, &s.Brand, &s.Category, | ||
| &s.BuyBoxPrice, &s.EstimatedMargin, &s.BSRRank, &s.SellerCount, | ||
| &s.Reason, &s.Status, &s.DealID, &s.CreatedAt, &s.ResolvedAt); err != nil { | ||
| return nil, err | ||
| } | ||
| suggestions = append(suggestions, s) | ||
| } | ||
| return suggestions, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify scanRows does not currently check iteration errors.
rg -n "func \(r \*SuggestionRepo\) scanRows" internal/adapter/postgres/suggestion_repo.go -A30
rg -n "rows\.Err\(" internal/adapter/postgres/suggestion_repo.goRepository: alastrat/amazonagent
Length of output: 695
Add iterator error check after row scanning.
scanRows exits the loop without checking for errors that may occur during iteration. Database cursors and streams can fail partway through, and these failures must be caught. Update the interface to include Err() error and check rows.Err() after the loop completes.
Proposed fix
-func (r *SuggestionRepo) scanRows(rows interface{ Next() bool; Scan(...any) error }) ([]domain.DiscoverySuggestion, error) {
+func (r *SuggestionRepo) scanRows(rows interface {
+ Next() bool
+ Scan(...any) error
+ Err() error
+}) ([]domain.DiscoverySuggestion, error) {
var suggestions []domain.DiscoverySuggestion
for rows.Next() {
var s domain.DiscoverySuggestion
if err := rows.Scan(&s.ID, &s.TenantID, &s.StrategyVersionID, &s.GoalID, &s.ASIN, &s.Title, &s.Brand, &s.Category,
&s.BuyBoxPrice, &s.EstimatedMargin, &s.BSRRank, &s.SellerCount,
&s.Reason, &s.Status, &s.DealID, &s.CreatedAt, &s.ResolvedAt); err != nil {
return nil, err
}
suggestions = append(suggestions, s)
}
+ if err := rows.Err(); err != nil {
+ return nil, err
+ }
return suggestions, nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/adapter/postgres/suggestion_repo.go` around lines 125 - 136, The
scanRows function currently iterates rows but doesn't check for iterator errors;
update the parameter type to include Err() error (e.g., interface{ Next() bool;
Scan(...any) error; Err() error }) and after the for loop call rows.Err() and
return any error encountered, ensuring scanRows (in SuggestionRepo) returns that
error instead of silently succeeding when iteration fails.
Comment on lines
+36
to
+41
| if time.Now().After(account.ResetAt) { | ||
| if err := s.accounts.ResetMonthly(ctx, tenantID); err != nil { | ||
| slog.Warn("credits: failed to reset monthly", "tenant_id", tenantID, "error", err) | ||
| } | ||
| account, _ = s.accounts.Get(ctx, tenantID) | ||
| } |
There was a problem hiding this comment.
Error from re-fetch after reset is silently ignored.
Line 40 discards the error from the second Get call. If that fails, the function returns the stale account from before the reset, which may have outdated UsedThisMonth value.
🐛 Proposed fix
if time.Now().After(account.ResetAt) {
if err := s.accounts.ResetMonthly(ctx, tenantID); err != nil {
slog.Warn("credits: failed to reset monthly", "tenant_id", tenantID, "error", err)
}
- account, _ = s.accounts.Get(ctx, tenantID)
+ if refreshed, err := s.accounts.Get(ctx, tenantID); err == nil {
+ account = refreshed
+ }
}📝 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 time.Now().After(account.ResetAt) { | |
| if err := s.accounts.ResetMonthly(ctx, tenantID); err != nil { | |
| slog.Warn("credits: failed to reset monthly", "tenant_id", tenantID, "error", err) | |
| } | |
| account, _ = s.accounts.Get(ctx, tenantID) | |
| } | |
| if time.Now().After(account.ResetAt) { | |
| if err := s.accounts.ResetMonthly(ctx, tenantID); err != nil { | |
| slog.Warn("credits: failed to reset monthly", "tenant_id", tenantID, "error", err) | |
| } | |
| if refreshed, err := s.accounts.Get(ctx, tenantID); err == nil { | |
| account = refreshed | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/service/credit_service.go` around lines 36 - 41, The re-fetch after
ResetMonthly currently discards the error from s.accounts.Get and may return a
stale account; update the code in the block that checks account.ResetAt so that
the second call to s.accounts.Get(ctx, tenantID) assigns both account and err,
checks err, and handles it (e.g., return the error up or log and return) instead
of ignoring it; ensure this change touches the variables used (account, err)
alongside functions account.ResetAt, s.accounts.ResetMonthly and s.accounts.Get
so UsedThisMonth is read from a fresh, validated account.
Tracked from the simplify review of PR #2. Prioritized by risk: - TD-1: Unify Brand/SharedBrand (high — data divergence risk) - TD-2: Batch INSERT for repos (high — 300 round-trips per assessment) - TD-3: Parallel SP-API in assessment (medium — 85s → 17s) - TD-4: QueryBuilder helper (low — prevents placeholder bugs) - TD-5: Unify product types (low — field drift) - TD-6: Credit reset cron (low — write-on-read) Each item has: problem, fix, files affected, effort estimate, risk. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Decision: multi-row INSERT ON CONFLICT for all batch operations. CopyFrom is faster at 50K+ rows but our max batch is 300 — the 6ms vs 30ms difference is imperceptible. CopyFrom requires a temp table workaround for upsert (4 SQL statements + txn management) vs 1 statement with multi-row INSERT ON CONFLICT. Revisit CopyFrom if we hit 50K+ row batches at scale. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
docs/superpowers/plans/2026-04-09-tech-debt-backlog.md (1)
185-199: Unify QueryBuilder constructor naming in the example.The API sketch defines
NewQueryBuilder(...)(Line 185), while usage callspostgres.NewQuery(...)(Line 194). Keep one constructor name to avoid copy/paste implementation drift across repos.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/superpowers/plans/2026-04-09-tech-debt-backlog.md` around lines 185 - 199, The example shows two different constructor names; update the usage or the API sketch so they match: choose either NewQueryBuilder or NewQuery and make the other references consistent (e.g., change the API declaration from func NewQueryBuilder(...) to func NewQuery(...) or update the usage to postgres.NewQueryBuilder(...)), and ensure any helper/factory functions and documentation examples (the usage block using postgres.NewQuery) reference the same constructor symbol to prevent implementation drift.
🤖 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/superpowers/plans/2026-04-09-tech-debt-backlog.md`:
- Line 297: The final summary's round-trip claim is inconsistent: update the
phrase "1 round-trip" in the TD-2 + TD-3 sentence to match the earlier estimate
of "6 round-trips" for 300 rows (or adjust the earlier estimate and all
dependent numbers if you intended a single round-trip); locate the text
containing "TD-2 + TD-3" and the phrase "1 round-trip" and replace it with the
corrected round-trip count and corresponding timing so the summary aligns with
the "6 chunks of 50 = 6 round-trips" estimate.
---
Nitpick comments:
In `@docs/superpowers/plans/2026-04-09-tech-debt-backlog.md`:
- Around line 185-199: The example shows two different constructor names; update
the usage or the API sketch so they match: choose either NewQueryBuilder or
NewQuery and make the other references consistent (e.g., change the API
declaration from func NewQueryBuilder(...) to func NewQuery(...) or update the
usage to postgres.NewQueryBuilder(...)), and ensure any helper/factory functions
and documentation examples (the usage block using postgres.NewQuery) reference
the same constructor symbol to prevent implementation drift.
🪄 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: 32fceef6-e555-4a87-9b51-aab936a65386
📒 Files selected for processing (1)
docs/superpowers/plans/2026-04-09-tech-debt-backlog.md
Security: - strategy_version_repo SetStatus: tenant-scoped (WHERE tenant_id) - suggestion_repo Accept/Dismiss: guard status transitions (only pending → accepted/dismissed, no double-accept) Correctness: - credit_repo Debit: reject non-positive amounts (prevents credit minting) - strategy_version_repo: handle JSON marshal/unmarshal errors - brand_catalog_repo + strategy_version_repo: add rows.Err() checks - credit_service GetBalance: handle re-fetch error after monthly reset - shared_catalog_service: move spapi nil check BEFORE credit spend (prevents wasting credits when SP-API is unavailable) Performance — multi-row INSERT batch helper: - New postgres.BatchInsert and postgres.BatchUpsert helpers - Chunks at 50 rows per statement (within Postgres param limits) - Refactored: SaveProbeResults (300→6 round-trips), SaveCategoryEligibilities (30→1), SuggestionRepo.CreateBatch (20→1), TenantEligibilityRepo.SetBatch (N→N/50) 269 tests still passing. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Transforms the platform from a product scanner into an AI concierge that assesses each seller's unique situation, generates a versioned growth strategy, and proactively discovers products aligned with approved goals.
55 files changed, +9,163 lines, 269 tests (all >90% coverage)
What's Built
Phase 0: Shared Catalog + Credit System
product_catalog,brand_catalog,tenant_product_eligibility,tenant_product_margins,credit_accounts,credit_transactionsPhase 1: Seller Profile + Eligibility Assessment
Phase 2: Strategy Versioning + Rollback
Phase 3: Daily Discovery Queue
Phase 4: Onboarding Frontend
New API Endpoints
Key Decisions Encoded
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation