More Demo Fixes Overall#279
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughMoves operator email into the canonical User record, normalizes and upserts it, removes operator.email usage, derives merchant billing email for /v1/buy, adds cap-hierarchy validation (backend + frontend), defers merchant client init using a new internal key config, and updates docs for buy/discover skills. ChangesEmail Architecture and Validation Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (2.12.2)level=error msg="[linters_context] typechecking error: pattern ./...: directory prefix . does not contain main module or its selected dependencies" Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
dbSchema/schema.ts (1)
53-69: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winEnforce canonical email values in the schema.
UpsertUserTxnormalizes email, butusers.emailstill accepts blank, untrimmed, or mixed-case values if any writer bypasses that helper. Since this column is now the canonical merchant-contact source, the table should reject non-canonical values instead of relying only on application code.As per coding guidelines, "Enforce correctness at the database layer (NOT NULL, unique, FK, CHECK constraints). Never rely on caller discipline."Suggested schema guard
-export const users = pgTable("users", { - workosId: text("workos_id").primaryKey(), - owner: text().notNull(), - email: varchar({ length: 320 }), // RFC 5321 max - createdAt: timestamp("created_at", { withTimezone: true }) - .notNull() - .defaultNow(), - updatedAt: timestamp("updated_at", { withTimezone: true }) - .notNull() - .defaultNow(), -}); +export const users = pgTable( + "users", + { + workosId: text("workos_id").primaryKey(), + owner: text().notNull(), + email: varchar({ length: 320 }), // RFC 5321 max + createdAt: timestamp("created_at", { withTimezone: true }) + .notNull() + .defaultNow(), + updatedAt: timestamp("updated_at", { withTimezone: true }) + .notNull() + .defaultNow(), + }, + (table) => [ + check( + "chk_users_email_canonical", + sql`${table.email} IS NULL OR ( + ${table.email} = lower(btrim(${table.email})) + AND length(btrim(${table.email})) > 0 + )`, + ), + ], +);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@dbSchema/schema.ts` around lines 53 - 69, The users.email column currently allows blank/untrimmed/mixed-case values; add DB-level guards so the table enforces canonical emails: make users.email NOT NULL and add a CHECK constraint on users (email = lower(trim(email)) AND email <> '') (optionally also validate format with an appropriate regex if desired). Update any migration/DDL that defines users (and reference UpsertUserTx behavior) so the new NOT NULL + CHECK are applied atomically during migration to avoid violating existing rows (e.g., backfill/trim existing null/blank values first).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@dbSchema/schema.ts`:
- Around line 53-69: The users.email column currently allows
blank/untrimmed/mixed-case values; add DB-level guards so the table enforces
canonical emails: make users.email NOT NULL and add a CHECK constraint on users
(email = lower(trim(email)) AND email <> '') (optionally also validate format
with an appropriate regex if desired). Update any migration/DDL that defines
users (and reference UpsertUserTx behavior) so the new NOT NULL + CHECK are
applied atomically during migration to avoid violating existing rows (e.g.,
backfill/trim existing null/blank values first).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 264160b6-8ef0-4102-b93c-25d98db6cc50
📒 Files selected for processing (15)
SANGRIA_BUY_SKILL.mdSANGRIA_DISCOVER_SKILL.mdbackend/adminHandlers/merchants.gobackend/auth/workos.gobackend/buyHandlers/buy.gobackend/clientHandlers/apiKeys.gobackend/clientHandlers/context.gobackend/dbEngine/agentOperators.gobackend/dbEngine/models.gobackend/dbEngine/organizations.gobackend/dbEngine/users.godbSchema/schema.tsfrontend/app/(client)/client/settings/ClientSettingsContent.tsxfrontend/components/CardSettingsModal.tsxfrontend/components/CreateAgentKeyModal.tsx
💤 Files with no reviewable changes (1)
- backend/clientHandlers/context.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/sangriamerchant/client.go`:
- Around line 274-281: The Buy flow currently attaches the shared secret
(c.internalKey) to req.Header in client.go regardless of the buyURL scheme; fix
this by resolving the buy URL via DeriveBuyURL (or using the already-resolved
buyURL) and only setting Authorization: Bearer <c.internalKey> when the URL
scheme is "https" — otherwise omit the header (or return an error/reject the
request) to avoid sending secrets over plaintext HTTP; update the logic around
c.internalKey and req.Header.Set to inspect the resolved URL.Scheme before
attaching the header.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ae0641be-d13e-4414-bf1e-41bbcd4565de
📒 Files selected for processing (4)
backend/buyHandlers/common.gobackend/config/sangriaInternal.gobackend/main.gobackend/sangriamerchant/client.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/x402Handlers/facilitator.go`:
- Around line 145-153: The current substring check using
strings.Contains(facilitatorURL, "api.cdp.coinbase.com") is too permissive and
can leak JWTs; instead parse facilitatorURL with url.Parse (or use net/url) and
do an exact hostname comparison (u.Hostname() == "api.cdp.coinbase.com"), handle
parse errors by returning nil, and keep the existing scheme check using
req.URL.Scheme != "https" (or additionally verify the parsed URL's Scheme if
appropriate) before attaching the CDP Authorization header in facilitator.go.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ab9a7aba-b06e-4fe9-a595-84956f404afa
📒 Files selected for processing (2)
backend/sangriamerchant/client.gobackend/x402Handlers/facilitator.go
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/buyHandlers/discover.go (1)
108-131: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winConsider skipping products with empty Name for consistency with price validation.
The new validation in
validateDiscoveryMatches(discoveries.go lines 142-144) will reject matches with emptyName. However, the handler only checksPriceUSDbefore building matches. If a merchant product has a valid price but an empty name, it will pass the handler's checks, be added to matches, and then causeCreateDiscoveryto fail withErrInvalidMatchShape—resulting in a 500 for the entire request even if other matches are valid.Adding a similar skip for empty Name would maintain consistent behavior: bad catalog entries are skipped with a warning, and valid matches can still proceed.
♻️ Proposed fix to add Name validation
for _, cand := range top { if cand.Product.PriceUSD == nil { slog.Warn("merchant product has invalid price; skipping candidate", "sku", cand.Product.SKU, "price_usd", "null") continue } + if strings.TrimSpace(cand.Product.Name) == "" { + slog.Warn("merchant product has empty name; skipping candidate", + "sku", cand.Product.SKU) + continue + } subtotal, ok := toMicrounitsSafe(*cand.Product.PriceUSD)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/buyHandlers/discover.go` around lines 108 - 131, The handler currently only skips candidates with invalid PriceUSD; add a check to also skip candidates with an empty product name to avoid creating invalid matches that later cause CreateDiscovery/validateDiscoveryMatches to return ErrInvalidMatchShape. In the loop that builds matches (where toMicrounitsSafe is called and matches = append(matches, dbengine.DiscoveryMatch{...})), verify cand.Product.Name != "" (or trim and check length) before computing subtotal; if empty, log a warning similar to the price checks (include "sku" and "name" fields) and continue so only matches with non-empty Name and valid price are appended.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/buyHandlers/discover.go`:
- Around line 108-131: The handler currently only skips candidates with invalid
PriceUSD; add a check to also skip candidates with an empty product name to
avoid creating invalid matches that later cause
CreateDiscovery/validateDiscoveryMatches to return ErrInvalidMatchShape. In the
loop that builds matches (where toMicrounitsSafe is called and matches =
append(matches, dbengine.DiscoveryMatch{...})), verify cand.Product.Name != ""
(or trim and check length) before computing subtotal; if empty, log a warning
similar to the price checks (include "sku" and "name" fields) and continue so
only matches with non-empty Name and valid price are appended.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: aaa9d0f3-d9d8-48e2-8420-a9319f5de9b7
📒 Files selected for processing (4)
backend/buyHandlers/discover.gobackend/dbEngine/discoveries.gobackend/dbEngine/models.gobackend/sangriamerchant/client.go
farhan-navas
force-pushed
the
farhan-navas/san-144-more-demo-fixes
branch
from
ca42250 to
50f8fc6
Compare
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 by CodeRabbit
Bug Fixes
Documentation