Update commerce dependencies and configuration#1
Merged
Conversation
…hic taxonomies, and seo
…le to use string-based step resolution
|
Need the big picture first? Review this PR in Change Stack to see what changed before going file by file. 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:
📝 WalkthroughWalkthroughAdds a richer product domain (variants/options/images/details/SEO), a new taxonomy module with repository and module wiring, migrates many tests to mdktest with in-memory DBs, refactors cart workflow handler registration, and adds changelog/security/review/CI adjustments. ChangesProduct Domain Evolution
Search and SEO Support
Taxonomy System
Test Infrastructure and Supporting Changes
Sequence DiagramsequenceDiagram
participant Client
participant GraphQLResolver
participant ProductHandlers
participant Repository
participant Database
Client->>GraphQLResolver: mutation CreateProduct(input)
GraphQLResolver->>ProductHandlers: workflowInput (map via JSON)
ProductHandlers->>ProductHandlers: ValidateProduct -> PersistProduct
ProductHandlers->>Repository: Save(product with associations)
Repository->>Database: INSERT/UPDATE product + related tables
Repository-->>ProductHandlers: persisted Product (with IDs/relations)
ProductHandlers-->>GraphQLResolver: workflow result
GraphQLResolver-->>Client: GraphQL response (created product)
🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cart/module.go (1)
28-67:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFail fast on workflow/handler registration errors.
Line 28 through Line 67 discard registration errors. If any register call fails, cart workflows can initialize partially and break later at execution time (e.g.,
cart.add/cart.remove/cart.checkoutGraphQL paths).Suggested fix
import ( "context" + "fmt" "github.com/GoHyperrr/mdk" ) @@ - _ = rt.Workflows().Register(mdk.Workflow{ + if err := rt.Workflows().Register(mdk.Workflow{ ID: "cart.add", Name: "Cart Add Item", Steps: []mdk.Step{ @@ }, - }) + }); err != nil { + return fmt.Errorf("register workflow cart.add: %w", err) + } @@ - _ = rt.Workflows().Register(mdk.Workflow{ + if err := rt.Workflows().Register(mdk.Workflow{ ID: "cart.remove", @@ - }) + }); err != nil { + return fmt.Errorf("register workflow cart.remove: %w", err) + } @@ - _ = rt.Workflows().Register(mdk.Workflow{ + if err := rt.Workflows().Register(mdk.Workflow{ ID: "cart.checkout", @@ - }) + }); err != nil { + return fmt.Errorf("register workflow cart.checkout: %w", err) + } - _ = rt.Workflows().RegisterHandler("cart.add_item", m.AddItemStep) - _ = rt.Workflows().RegisterHandler("cart.remove_item", m.RemoveItemStep) - _ = rt.Workflows().RegisterHandler("cart.checkout", m.CheckoutStep) + if err := rt.Workflows().RegisterHandler("cart.add_item", m.AddItemStep); err != nil { + return fmt.Errorf("register handler cart.add_item: %w", err) + } + if err := rt.Workflows().RegisterHandler("cart.remove_item", m.RemoveItemStep); err != nil { + return fmt.Errorf("register handler cart.remove_item: %w", err) + } + if err := rt.Workflows().RegisterHandler("cart.checkout", m.CheckoutStep); err != nil { + return fmt.Errorf("register handler cart.checkout: %w", err) + }🤖 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 `@cart/module.go` around lines 28 - 67, The workflow and handler registration calls currently ignore returned errors (rt.Workflows().Register and rt.Workflows().RegisterHandler), which can leave the cart module partially registered; update the init/registration code to check each error from Register for workflows "cart.add", "cart.remove", "cart.checkout" and from RegisterHandler for "cart.add_item" (m.AddItemStep), "cart.remove_item" (m.RemoveItemStep) and "cart.checkout" (m.CheckoutStep) and fail fast—propagate or return the error (or log and panic) so initialization aborts on any registration failure instead of discarding the error.product/handlers.go (1)
218-222:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate that
productIDis not empty before querying.Line 218 extracts
productIDusing type assertion with blank identifier for ok, but doesn't verify the string is non-empty. If the ID is missing or an empty string, the repository query on Line 219 will fail with a potentially confusing error message.Add validation
productID, _ := workflowInput["id"].(string) + if productID == "" { + return nil, fmt.Errorf("product ID is required") + } p, err := m.repo.GetByID(ctx, productID)🤖 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 `@product/handlers.go` around lines 218 - 222, The code reads productID from workflowInput via productID, _ := workflowInput["id"].(string) and calls m.repo.GetByID(ctx, productID) without validating it; update the handler to check the type assertion result and that productID is not an empty string (e.g., ensure the "ok" from workflowInput["id"].(string) is true and productID != "") and return a clear error (e.g., "missing product id" or validation error) before calling m.repo.GetByID so you don't query the repo with an empty ID.
🤖 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 `@CHANGELOG.md`:
- Line 7: Insert a blank line before the "### Added" heading in CHANGELOG.md so
the heading is separated by an empty line from the preceding content; update the
file to ensure the "### Added" line is surrounded by blank lines to follow
Markdown best practices.
In `@go.mod`:
- Line 27: The go.mod currently contains a local replace directive "replace
github.com/GoHyperrr/mdk => ../mdk" which forces builds to depend on a relative
path; remove this line from go.mod and either add the local replacement to a
go.work file or document using a local go.work for dev only so CI and other devs
use the canonical module; update references to the replace directive (the
"replace github.com/GoHyperrr/mdk => ../mdk" entry) and ensure go.mod is
committed without the local replace before merging.
In `@product/graphql.go`:
- Around line 40-48: The current conversion in product/graphql.go uses
json.Marshal followed by json.Unmarshal (variables inputBytes and workflowInput)
to turn the typed input into map[string]any; replace this inefficient round-trip
with a direct struct->map conversion (e.g., use a dedicated library such as
github.com/mitchellh/mapstructure or a small reflection helper) so you eliminate
the encode/decode overhead—locate the block that calls json.Marshal(input) and
json.Unmarshal(..., &workflowInput) and replace it with a direct conversion that
produces the same map[string]any for the workflow context.
In `@product/handlers.go`:
- Around line 128-138: The struct literal for p currently sets Description: ""
and then later mutates p.Description when in.Description != nil; instead set
Description directly in the literal (use a conditional/dereference of
in.Description or omit the field to rely on the zero value) so you can remove
the separate if block; update the struct literal where p is created (and remove
the subsequent if in.Description != nil { p.Description = *in.Description }).
- Around line 42-49: Add enum validation for Type and Status inside the existing
variants loop (the code iterating over in.Variants and checking v.Title and
v.Price). Ensure v.Type is one of ["PHYSICAL","DIGITAL","SERVICE"] and v.Status
is one of ["DRAFT","PUBLISHED","ARCHIVED"]; if not, return a descriptive error
via fmt.Errorf (e.g., "invalid variant type: %q" or "invalid variant status:
%q"). Place these checks alongside the existing v.Title and v.Price validations
so invalid Type/Status values are rejected before persistence.
In `@product/model.go`:
- Around line 104-124: The ProductVariant struct uses float64 for Price and
CompareAtPrice which risks monetary precision issues; change Price to int64
(cents) and CompareAtPrice to *int64, update the JSON tags to reflect cents
semantics (or implement custom marshal/unmarshal if you want JSON in dollars),
and update any code interacting with ProductVariant.Price/CompareAtPrice
(creation, updates, calculations, and DB queries) to convert between dollars and
cents; locate these fields on the ProductVariant type and adjust related
functions (e.g., BeforeCreate usage, any validators, services, or DB callers) to
use integer cents for safe arithmetic.
- Around line 19-28: The StringList.Value method currently returns the JSON
string "[]" for a nil receiver, losing the nil vs empty distinction; if you want
nil to map to SQL NULL, change StringList.Value (the method on type StringList)
to return (nil, nil) when sl == nil, and ensure the complementary
StringList.Scan implementation correctly handles sql NULLs (e.g., treats nil
source as nil slice) so the database can represent "no value" vs "explicitly
empty".
In `@product/product_test.go`:
- Around line 22-23: Replace the unchecked type assertion on rt.Workflows() with
the two-value form and fail the test with a clear message if the assertion
fails: get the value via runner, ok :=
rt.Workflows().(*mdktest.TestWorkflowEngine) and if !ok call t.Fatalf or
require.Failf with the actual type (e.g., using %T) so the test reports a clear
error instead of panicking; update the variable name runner and any downstream
usage accordingly.
In `@product/product.graphqls`:
- Around line 146-156: UpdateProductInput currently omits nested resources
(variants, options, images) so updateProduct cannot modify them; add explicit
nested update input fields (e.g., variants: [UpdateProductVariantInput],
options: [UpdateProductOptionInput], images: [UpdateProductImageInput]) to
UpdateProductInput or add separate mutations (e.g., updateProductVariant,
updateProductOption, replaceProductImages) and corresponding input types; ensure
the new input types mirror createProduct's nested inputs and include identifiers
for existing nested entities (ids) plus mutate operations (patch, replace, add,
remove) so Product consumers can adjust pricing/options/media after creation.
- Around line 25-27: The schema declares options, variants, images as non-null
lists but PersistProduct in product/handlers.go leaves Product.Options,
Product.Variants, Product.Images and ProductVariant.Options nil when omitted;
either (A) ensure PersistProduct (and any other product-returning functions)
always initializes those fields to empty slices (e.g., set Product.Options =
[]ProductOption{} etc., and ProductVariant.Options = []VariantOption{} before
returning) so resolvers never return nil, or (B) relax the GraphQL schema by
changing those fields from [Type!]! to [Type!] or [Type] as appropriate (also
adjust the field at the other location noted around line 64) so null/nil slices
are allowed. Ensure whichever approach you pick is applied consistently across
PersistProduct and any code paths that construct Product and ProductVariant
objects.
In `@product/repository.go`:
- Around line 54-64: The List method on Repository currently loads all Product
rows (Repository.List) which can OOM for large catalogs; change its signature to
accept pagination parameters (e.g., limit and offset or a cursor token) and
apply GORM's Limit/Offset (or cursor WHERE + Order) before Find, keep the same
Preload calls (Options, Variants, Variants.Options, Images), validate/clip the
limit to a sane max, and update any callers of Repository.List to pass the new
pagination arguments (or provide a wrapper that defaults to safe page sizes).
In `@search/search_test.go`:
- Around line 29-30: The unguarded type assertion on rt.Workflows() to
*mdktest.TestWorkflowEngine may panic; change it to the two-value form (e.g. w,
ok := rt.Workflows().(*mdktest.TestWorkflowEngine)) and if ok is false call the
test failure helper (t.Fatalf or t.Fatalf-like) with a clear message including
the actual type (use %T) so the test fails with a readable error instead of
panicking.
In `@taxonomy/repository.go`:
- Around line 119-121: In GetTermsForResource, when no relations are found
currently it returns nil, nil; change this to return an empty slice (e.g.,
[]TaxonomyTerm{}) and nil error to avoid nil/empty ambiguity downstream—update
the branch that checks len(relations) == 0 to return an empty TaxonomyTerm slice
instead of nil, preserving the function signature and behavior elsewhere.
- Around line 71-87: GetTermTree currently appends term values (append(roots,
*t)) so later updates to nodes in termMap (like adding children) don't propagate
into previously-copied ancestors; fix by building the tree using pointer links
from the termMap so mutations propagate deterministically: create termMap as
map[string]*Term, populate it with pointers to the original slice items, link
children by appending pointers (parent.Children = append(parent.Children,
child)) or by setting Children to slices of pointers, and finally collect roots
by referencing the map entries (roots = append(roots, *termMap[id]) or return
[]*Term) so all child additions are visible on ancestors; update
functions/variables GetTermTree, termMap, roots, and the loop that processes
ParentID/Children accordingly.
---
Outside diff comments:
In `@cart/module.go`:
- Around line 28-67: The workflow and handler registration calls currently
ignore returned errors (rt.Workflows().Register and
rt.Workflows().RegisterHandler), which can leave the cart module partially
registered; update the init/registration code to check each error from Register
for workflows "cart.add", "cart.remove", "cart.checkout" and from
RegisterHandler for "cart.add_item" (m.AddItemStep), "cart.remove_item"
(m.RemoveItemStep) and "cart.checkout" (m.CheckoutStep) and fail fast—propagate
or return the error (or log and panic) so initialization aborts on any
registration failure instead of discarding the error.
In `@product/handlers.go`:
- Around line 218-222: The code reads productID from workflowInput via
productID, _ := workflowInput["id"].(string) and calls m.repo.GetByID(ctx,
productID) without validating it; update the handler to check the type assertion
result and that productID is not an empty string (e.g., ensure the "ok" from
workflowInput["id"].(string) is true and productID != "") and return a clear
error (e.g., "missing product id" or validation error) before calling
m.repo.GetByID so you don't query the repo with an empty ID.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: ddf63db8-a7a0-4563-94bf-f143bb91bc3e
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (26)
CHANGELOG.mdSECURITY.mdanalytics/analytics_test.gocart/cart_test.gocart/module.gocart/repo_test.gocustomer/customer_test.gofinance/finance_test.gofulfillment/fulfillment_test.gogo.modmarketing/marketing_test.gonotification/notification_test.goorder/order_test.goproduct/graphql.goproduct/handlers.goproduct/model.goproduct/module.goproduct/product.graphqlsproduct/product_test.goproduct/repository.gosearch/search_test.goseo/seo.gosupport/support_test.gotaxonomy/model.gotaxonomy/module.gotaxonomy/repository.go
|
|
||
| ## [0.1.0] - 2026-06-05 | ||
|
|
||
| ### Added |
There was a problem hiding this comment.
Add blank line before heading.
The heading should be surrounded by blank lines per Markdown best practices.
📝 Proposed fix
## [0.1.0] - 2026-06-05
-
+
### Added🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 7-7: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 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 `@CHANGELOG.md` at line 7, Insert a blank line before the "### Added" heading
in CHANGELOG.md so the heading is separated by an empty line from the preceding
content; update the file to ensure the "### Added" line is surrounded by blank
lines to follow Markdown best practices.
Source: Linters/SAST tools
| modernc.org/sqlite v1.23.1 // indirect | ||
| ) | ||
|
|
||
| replace github.com/GoHyperrr/mdk => ../mdk |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check CI/build configs for references to local mdk path
# Search for references to ../mdk in CI and build files
rg -n --type=yaml --type=toml --type=json '../mdk' -g '!go.mod'
# Check if there are any build/CI workflow files
fd -e yml -e yaml . .github .gitlab-ci.yml .circleciRepository: GoHyperrr/commerce
Length of output: 222
Remove/relocate the local go.mod replace to avoid CI/other developers depending on ../mdk.
go.mod contains:
replace github.com/GoHyperrr/mdk => ../mdkGo will apply this replace during go build/go test regardless of whether CI/workflow files mention ../mdk directly, so the path must exist relative to the build workspace. Move the local linkage to a go.work/local-only setup or remove the directive before merge.
🤖 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 `@go.mod` at line 27, The go.mod currently contains a local replace directive
"replace github.com/GoHyperrr/mdk => ../mdk" which forces builds to depend on a
relative path; remove this line from go.mod and either add the local replacement
to a go.work file or document using a local go.work for dev only so CI and other
devs use the canonical module; update references to the replace directive (the
"replace github.com/GoHyperrr/mdk => ../mdk" entry) and ensure go.mod is
committed without the local replace before merging.
Comment on lines
+40
to
48
| // Convert input struct to map[string]any for workflow context | ||
| inputBytes, err := json.Marshal(input) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| workflowInput := map[string]any{ | ||
| "id": input.ID, | ||
| "name": input.Name, | ||
| "description": desc, | ||
| "price": input.Price, | ||
| var workflowInput map[string]any | ||
| if err := json.Unmarshal(inputBytes, &workflowInput); err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
JSON marshal/unmarshal round-trip is inefficient but acceptable.
The code uses json.Marshal then json.Unmarshal to convert typed input structs into map[string]any for workflow context. While this works correctly and handles nested structures, it incurs encoding/decoding overhead.
This pattern is common for type-safe conversion to dynamic maps and is acceptable unless profiling shows it's a bottleneck. If performance becomes a concern, consider using a library like mitchellh/mapstructure or implement direct struct-to-map conversion.
Also applies to: 80-88
🤖 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 `@product/graphql.go` around lines 40 - 48, The current conversion in
product/graphql.go uses json.Marshal followed by json.Unmarshal (variables
inputBytes and workflowInput) to turn the typed input into map[string]any;
replace this inefficient round-trip with a direct struct->map conversion (e.g.,
use a dedicated library such as github.com/mitchellh/mapstructure or a small
reflection helper) so you eliminate the encode/decode overhead—locate the block
that calls json.Marshal(input) and json.Unmarshal(..., &workflowInput) and
replace it with a direct conversion that produces the same map[string]any for
the workflow context.
Comment on lines
+42
to
49
| for _, v := range in.Variants { | ||
| if v.Title == "" { | ||
| return nil, fmt.Errorf("variant title is required") | ||
| } | ||
| if v.Price < 0 { | ||
| return nil, fmt.Errorf("variant price cannot be negative") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider validating Type and Status fields against allowed values.
The validation checks required fields and non-negative prices but doesn't verify that Type is one of ["PHYSICAL", "DIGITAL", "SERVICE"] or that Status is one of ["DRAFT", "PUBLISHED", "ARCHIVED"] (as implied by the model defaults). If invalid values are allowed to pass here, they'll be persisted as-is.
Add enum validation
if in.Handle == "" {
return nil, fmt.Errorf("product handle is required")
}
+
+ if in.Type != nil {
+ allowedTypes := map[string]bool{"PHYSICAL": true, "DIGITAL": true, "SERVICE": true}
+ if !allowedTypes[*in.Type] {
+ return nil, fmt.Errorf("invalid product type: must be PHYSICAL, DIGITAL, or SERVICE")
+ }
+ }
+ if in.Status != nil {
+ allowedStatuses := map[string]bool{"DRAFT": true, "PUBLISHED": true, "ARCHIVED": true}
+ if !allowedStatuses[*in.Status] {
+ return nil, fmt.Errorf("invalid product status: must be DRAFT, PUBLISHED, or ARCHIVED")
+ }
+ }
for _, v := range in.Variants {📝 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
| for _, v := range in.Variants { | |
| if v.Title == "" { | |
| return nil, fmt.Errorf("variant title is required") | |
| } | |
| if v.Price < 0 { | |
| return nil, fmt.Errorf("variant price cannot be negative") | |
| } | |
| } | |
| if in.Handle == "" { | |
| return nil, fmt.Errorf("product handle is required") | |
| } | |
| if in.Type != nil { | |
| allowedTypes := map[string]bool{"PHYSICAL": true, "DIGITAL": true, "SERVICE": true} | |
| if !allowedTypes[*in.Type] { | |
| return nil, fmt.Errorf("invalid product type: must be PHYSICAL, DIGITAL, or SERVICE") | |
| } | |
| } | |
| if in.Status != nil { | |
| allowedStatuses := map[string]bool{"DRAFT": true, "PUBLISHED": true, "ARCHIVED": true} | |
| if !allowedStatuses[*in.Status] { | |
| return nil, fmt.Errorf("invalid product status: must be DRAFT, PUBLISHED, or ARCHIVED") | |
| } | |
| } | |
| for _, v := range in.Variants { | |
| if v.Title == "" { | |
| return nil, fmt.Errorf("variant title is required") | |
| } | |
| if v.Price < 0 { | |
| return nil, fmt.Errorf("variant price cannot be negative") | |
| } | |
| } |
🤖 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 `@product/handlers.go` around lines 42 - 49, Add enum validation for Type and
Status inside the existing variants loop (the code iterating over in.Variants
and checking v.Title and v.Price). Ensure v.Type is one of
["PHYSICAL","DIGITAL","SERVICE"] and v.Status is one of
["DRAFT","PUBLISHED","ARCHIVED"]; if not, return a descriptive error via
fmt.Errorf (e.g., "invalid variant type: %q" or "invalid variant status: %q").
Place these checks alongside the existing v.Title and v.Price validations so
invalid Type/Status values are rejected before persistence.
Comment on lines
+128
to
+138
| Description: "", | ||
| Type: pType, | ||
| Status: pStatus, | ||
| Details: pDetails, | ||
| SEO: pSEO, | ||
| Metadata: in.Metadata, | ||
| AISystemContext: pAISystemContext, | ||
| } | ||
| if in.Description != nil { | ||
| p.Description = *in.Description | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Description defaults to empty string even when provided.
Line 128 initializes Description: "", then Lines 136-138 check if in.Description != nil to set it. This is correct, but you could simplify by directly setting Description in the struct literal with a ternary-style approach or omit the default since zero-value for string is already "".
Minor simplification
+ pDescription := ""
+ if in.Description != nil {
+ pDescription = *in.Description
+ }
+
p := &Product{
ID: in.ID,
Name: in.Name,
Handle: in.Handle,
- Description: "",
+ Description: pDescription,
Type: pType,
Status: pStatus,
Details: pDetails,
SEO: pSEO,
Metadata: in.Metadata,
AISystemContext: pAISystemContext,
}
- if in.Description != nil {
- p.Description = *in.Description
- }🤖 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 `@product/handlers.go` around lines 128 - 138, The struct literal for p
currently sets Description: "" and then later mutates p.Description when
in.Description != nil; instead set Description directly in the literal (use a
conditional/dereference of in.Description or omit the field to rely on the zero
value) so you can remove the separate if block; update the struct literal where
p is created (and remove the subsequent if in.Description != nil { p.Description
= *in.Description }).
Comment on lines
146
to
156
| input UpdateProductInput { | ||
| name: String | ||
| handle: String | ||
| description: String | ||
| price: Float | ||
| currency: String | ||
| type: String | ||
| status: String | ||
| details: ProductDetailsInput | ||
| seo: SEOInput | ||
| metadata: Map | ||
| aiSystemContext: String | ||
| } |
There was a problem hiding this comment.
updateProduct cannot modify variants, options, or images.
Product exposes these nested resources, and createProduct accepts them, but UpdateProductInput omits them entirely. After creation, clients have no GraphQL path to fix variant pricing/options or replace media. Add explicit nested update inputs here, or separate mutations for those associations, before shipping this contract.
🤖 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 `@product/product.graphqls` around lines 146 - 156, UpdateProductInput
currently omits nested resources (variants, options, images) so updateProduct
cannot modify them; add explicit nested update input fields (e.g., variants:
[UpdateProductVariantInput], options: [UpdateProductOptionInput], images:
[UpdateProductImageInput]) to UpdateProductInput or add separate mutations
(e.g., updateProductVariant, updateProductOption, replaceProductImages) and
corresponding input types; ensure the new input types mirror createProduct's
nested inputs and include identifiers for existing nested entities (ids) plus
mutate operations (patch, replace, add, remove) so Product consumers can adjust
pricing/options/media after creation.
Comment on lines
+54
to
64
| // List retrieves all products with all of their relationships preloaded. | ||
| func (r *Repository) List(ctx context.Context) ([]*Product, error) { | ||
| var products []*Product | ||
| err := r.db.WithContext(ctx).Find(&products).Error | ||
| err := r.db.WithContext(ctx). | ||
| Preload("Options"). | ||
| Preload("Variants"). | ||
| Preload("Variants.Options"). | ||
| Preload("Images"). | ||
| Find(&products).Error | ||
| return products, err | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider adding pagination to List to handle large product catalogs.
Line 62 fetches all products without pagination. For a growing catalog, this could cause memory pressure and slow response times. Consider adding limit/offset or cursor-based pagination parameters to the List method.
Example with limit/offset
func (r *Repository) List(ctx context.Context, limit, offset int) ([]*Product, error) {
var products []*Product
err := r.db.WithContext(ctx).
Preload("Options").
Preload("Variants").
Preload("Variants.Options").
Preload("Images").
Limit(limit).
Offset(offset).
Find(&products).Error
return products, err
}🤖 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 `@product/repository.go` around lines 54 - 64, The List method on Repository
currently loads all Product rows (Repository.List) which can OOM for large
catalogs; change its signature to accept pagination parameters (e.g., limit and
offset or a cursor token) and apply GORM's Limit/Offset (or cursor WHERE +
Order) before Find, keep the same Preload calls (Options, Variants,
Variants.Options, Images), validate/clip the limit to a sane max, and update any
callers of Repository.List to pass the new pagination arguments (or provide a
wrapper that defaults to safe page sizes).
Comment on lines
29
to
+30
| _ = database.AutoMigrate(models...) | ||
| runner := rt.Workflows().(*mdk.TestWorkflowEngine) | ||
| runner := rt.Workflows().(*mdktest.TestWorkflowEngine) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Type assertion without ok-check could panic.
Line 30 performs a type assertion without checking the ok boolean. If rt.Workflows() returns a different type, this will panic. While this is test code and panics are acceptable, using the two-value form would provide a clearer error message.
Safer assertion
_ = database.AutoMigrate(models...)
- runner := rt.Workflows().(*mdktest.TestWorkflowEngine)
+ runner, ok := rt.Workflows().(*mdktest.TestWorkflowEngine)
+ if !ok {
+ t.Fatal("expected *mdktest.TestWorkflowEngine")
+ }📝 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
| _ = database.AutoMigrate(models...) | |
| runner := rt.Workflows().(*mdk.TestWorkflowEngine) | |
| runner := rt.Workflows().(*mdktest.TestWorkflowEngine) | |
| _ = database.AutoMigrate(models...) | |
| runner, ok := rt.Workflows().(*mdktest.TestWorkflowEngine) | |
| if !ok { | |
| t.Fatal("expected *mdktest.TestWorkflowEngine") | |
| } |
🤖 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 `@search/search_test.go` around lines 29 - 30, The unguarded type assertion on
rt.Workflows() to *mdktest.TestWorkflowEngine may panic; change it to the
two-value form (e.g. w, ok := rt.Workflows().(*mdktest.TestWorkflowEngine)) and
if ok is false call the test failure helper (t.Fatalf or t.Fatalf-like) with a
clear message including the actual type (use %T) so the test fails with a
readable error instead of panicking.
Comment on lines
+71
to
+87
| for i := range terms { | ||
| t := &terms[i] | ||
| if t.ParentID == nil || *t.ParentID == "" { | ||
| roots = append(roots, *t) | ||
| } else { | ||
| if parent, exists := termMap[*t.ParentID]; exists { | ||
| parent.Children = append(parent.Children, *t) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Re-map children updates to root elements | ||
| for i := range roots { | ||
| if updated, exists := termMap[roots[i].ID]; exists { | ||
| roots[i] = *updated | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix non-deterministic tree construction in GetTermTree.
Line 77 appends *t by value, so later child mutations on t are not guaranteed to propagate into already-copied ancestors. This makes deeper trees order-dependent and can drop descendants; missing parents are also silently discarded.
Suggested rewrite for deterministic recursive materialization
- termMap := make(map[string]*TaxonomyTerm)
- var roots []TaxonomyTerm
+ termMap := make(map[string]*TaxonomyTerm, len(terms))
+ childrenByParent := make(map[string][]*TaxonomyTerm)
+ var roots []*TaxonomyTerm
for i := range terms {
- termMap[terms[i].ID] = &terms[i]
+ t := &terms[i]
+ t.Children = nil
+ termMap[t.ID] = t
}
for i := range terms {
t := &terms[i]
if t.ParentID == nil || *t.ParentID == "" {
- roots = append(roots, *t)
+ roots = append(roots, t)
} else {
- if parent, exists := termMap[*t.ParentID]; exists {
- parent.Children = append(parent.Children, *t)
- }
+ if _, exists := termMap[*t.ParentID]; !exists {
+ // Keep orphaned terms visible instead of dropping them.
+ roots = append(roots, t)
+ continue
+ }
+ childrenByParent[*t.ParentID] = append(childrenByParent[*t.ParentID], t)
}
}
- // Re-map children updates to root elements
- for i := range roots {
- if updated, exists := termMap[roots[i].ID]; exists {
- roots[i] = *updated
- }
- }
+ var build func(node *TaxonomyTerm) TaxonomyTerm
+ build = func(node *TaxonomyTerm) TaxonomyTerm {
+ out := *node
+ kids := childrenByParent[node.ID]
+ out.Children = make([]TaxonomyTerm, 0, len(kids))
+ for _, child := range kids {
+ out.Children = append(out.Children, build(child))
+ }
+ return out
+ }
+
+ result := make([]TaxonomyTerm, 0, len(roots))
+ for _, root := range roots {
+ result = append(result, build(root))
+ }
- return roots, nil
+ return result, nil🤖 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 `@taxonomy/repository.go` around lines 71 - 87, GetTermTree currently appends
term values (append(roots, *t)) so later updates to nodes in termMap (like
adding children) don't propagate into previously-copied ancestors; fix by
building the tree using pointer links from the termMap so mutations propagate
deterministically: create termMap as map[string]*Term, populate it with pointers
to the original slice items, link children by appending pointers
(parent.Children = append(parent.Children, child)) or by setting Children to
slices of pointers, and finally collect roots by referencing the map entries
(roots = append(roots, *termMap[id]) or return []*Term) so all child additions
are visible on ancestors; update functions/variables GetTermTree, termMap,
roots, and the loop that processes ParentID/Children accordingly.
Comment on lines
+119
to
+121
| if len(relations) == 0 { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Return an empty slice instead of nil for no linked terms.
GetTermsForResource currently returns nil, nil; returning []TaxonomyTerm{} avoids downstream null/empty ambiguity.
Small contract-consistency fix
if len(relations) == 0 {
- return nil, nil
+ return []TaxonomyTerm{}, 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
| if len(relations) == 0 { | |
| return nil, nil | |
| } | |
| if len(relations) == 0 { | |
| return []TaxonomyTerm{}, nil | |
| } |
🤖 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 `@taxonomy/repository.go` around lines 119 - 121, In GetTermsForResource, when
no relations are found currently it returns nil, nil; change this to return an
empty slice (e.g., []TaxonomyTerm{}) and nil error to avoid nil/empty ambiguity
downstream—update the branch that checks len(relations) == 0 to return an empty
TaxonomyTerm slice instead of nil, preserving the function signature and
behavior elsewhere.
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 @.coderabbit.yaml:
- Line 3: The schema URL in .coderabbit.yaml currently points to
https://coderabbit.ai/integrations/schema.v2.json (which is valid for
validation) but the human-facing docs link referenced in your config points to a
404 at https://docs.coderabbit.ai/spec/configuration; update that documentation
URL to the correct, reachable docs page (or remove the dead docs reference) so
users can follow the spec; check any occurrences of reviews.tone_instructions in
the file remain valid per the schema after updating the URL.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c6f65b6b-f2a3-463a-bb89-032a138b52f5
📒 Files selected for processing (1)
.coderabbit.yaml
| @@ -1,17 +1,16 @@ | |||
| # CodeRabbit Configuration File | |||
| # Reference: https://docs.coderabbit.ai/spec/configuration | |||
| # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json | |||
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify schema URL is accessible and validate configuration properties
# Check if schema URL returns valid JSON
echo "Fetching schema from URL..."
curl -s -f "https://coderabbit.ai/integrations/schema.v2.json" -o /tmp/coderabbit-schema.json
if [ $? -eq 0 ]; then
echo "✓ Schema URL is accessible"
echo "Schema content preview:"
cat /tmp/coderabbit-schema.json | jq -r 'keys' 2>/dev/null || cat /tmp/coderabbit-schema.json | head -20
else
echo "✗ Schema URL is not accessible or returned an error"
fi
# Check documentation URL
echo -e "\nChecking documentation URL..."
curl -s -f -I "https://docs.coderabbit.ai/spec/configuration" > /dev/null
if [ $? -eq 0 ]; then
echo "✓ Documentation URL is accessible"
else
echo "✗ Documentation URL is not accessible"
fiRepository: GoHyperrr/commerce
Length of output: 377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Checking schema URL response (follow redirects) =="
SCHEMA_URL="https://coderabbit.ai/integrations/schema.v2.json"
# Capture headers and status code
curl -sS -L -o /tmp/coderabbit-schema-raw.json \
-w "\nHTTP_STATUS=%{http_code}\nCONTENT_TYPE=%{content_type}\n" \
"$SCHEMA_URL" | sed -n '1,3p'
# Quick sanity: does it look like JSON?
head -c 200 /tmp/coderabbit-schema-raw.json; echo
python3 - <<'PY'
import json
p="/tmp/coderabbit-schema-raw.json"
try:
with open(p,"r",encoding="utf-8",errors="replace") as f:
data=f.read().lstrip()
json.loads(data)
print("JSON_PARSE=ok")
except Exception as e:
print("JSON_PARSE=fail:", type(e).__name__, str(e)[:200])
PY
echo
echo "== Checking documentation URL response =="
DOC_URL="https://docs.coderabbit.ai/spec/configuration"
curl -sS -L -I "$DOC_URL" | head -n 20Repository: GoHyperrr/commerce
Length of output: 2084
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .coderabbit.yaml (relevant lines) =="
nl -ba .coderabbit.yaml | sed -n '1,120p'
echo
echo "== Validate .coderabbit.yaml against schema =="
python3 - <<'PY'
import os, sys, json, urllib.request
from urllib.error import HTTPError, URLError
# Install deps if missing (sandbox usually allows pip installs)
import pkgutil, subprocess
def ensure(pkg, import_name=None):
if import_name is None: import_name = pkg
if pkgutil.find_loader(import_name) is not None:
return
subprocess.check_call([sys.executable, "-m", "pip", "install", pkg])
ensure("pyyaml", "yaml")
ensure("jsonschema", "jsonschema")
import yaml
from jsonschema import Draft202012Validator
schema_url = "https://coderabbit.ai/integrations/schema.v2.json"
schema_raw = urllib.request.urlopen(schema_url).read().decode("utf-8", errors="replace")
schema = json.loads(schema_raw)
with open(".coderabbit.yaml","r",encoding="utf-8") as f:
cfg_raw = f.read()
# yaml-language-server directive is a YAML comment; parse YAML anyway
cfg = yaml.safe_load(cfg_raw) or {}
# Some YAML files may parse into non-dict; normalize
if not isinstance(cfg, dict):
print("CONFIG_PARSE=non-object:", type(cfg).__name__)
sys.exit(0)
validator = Draft202012Validator(schema)
errors = sorted(validator.iter_errors(cfg), key=lambda e: list(e.path))
print("CONFIG_KEYS=", sorted(cfg.keys()))
print("SCHEMA_HAS_tone_instructions=", "tone_instructions" in (schema.get("properties") or {}))
print("SCHEMA_HAS_tone_instruction=", "tone_instruction" in (schema.get("properties") or {}))
if errors:
print(f"VALIDATION=FAIL errors={len(errors)}")
for e in errors[:20]:
path = ".".join(str(p) for p in e.path) if e.path else "(root)"
print(f"- path={path} validator={e.validator} message={e.message}")
else:
print("VALIDATION=OK")
PYRepository: GoHyperrr/commerce
Length of output: 163
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== .coderabbit.yaml (relevant lines) =="
cat -n .coderabbit.yaml | sed -n '1,200p'
echo
echo "== Validate .coderabbit.yaml against schema =="
python3 - <<'PY'
import sys, json, urllib.request, yaml
from jsonschema import Draft202012Validator
schema_url = "https://coderabbit.ai/integrations/schema.v2.json"
schema_raw = urllib.request.urlopen(schema_url).read().decode("utf-8", errors="replace")
schema = json.loads(schema_raw)
with open(".coderabbit.yaml","r",encoding="utf-8") as f:
cfg_raw = f.read()
cfg = yaml.safe_load(cfg_raw) or {}
if not isinstance(cfg, dict):
print("CONFIG_PARSE=non-object:", type(cfg).__name__)
sys.exit(0)
validator = Draft202012Validator(schema)
errors = sorted(validator.iter_errors(cfg), key=lambda e: list(e.path))
props = schema.get("properties") or {}
print("CONFIG_KEYS=", sorted(cfg.keys()))
print("SCHEMA_HAS_tone_instructions=", "tone_instructions" in props)
print("SCHEMA_HAS_tone_instruction=", "tone_instruction" in props)
if errors:
print(f"VALIDATION=FAIL errors={len(errors)}")
for e in errors[:30]:
path = ".".join(str(p) for p in e.path) if e.path else "(root)"
print(f"- path={path} validator={e.validator} message={e.message}")
else:
print("VALIDATION=OK")
PYRepository: GoHyperrr/commerce
Length of output: 3361
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SCHEMA_URL="https://coderabbit.ai/integrations/schema.v2.json"
SCHEMA_PATH="/tmp/coderabbit-schema.json"
echo "== Fetch schema via curl (bypass python SSL) =="
curl -sS -L "$SCHEMA_URL" -o "$SCHEMA_PATH"
python3 - <<'PY'
import sys, json, yaml, os
from jsonschema import Draft202012Validator
schema_path="/tmp/coderabbit-schema.json"
with open(schema_path,"r",encoding="utf-8") as f:
schema=json.load(f)
with open(".coderabbit.yaml","r",encoding="utf-8") as f:
cfg=yaml.safe_load(f) or {}
if not isinstance(cfg, dict):
print("CONFIG_PARSE=non-object:", type(cfg).__name__)
sys.exit(0)
validator=Draft202012Validator(schema)
errors=sorted(validator.iter_errors(cfg), key=lambda e: list(e.path))
# Check for tone_instructions location in schema (best-effort)
props=schema.get("properties") or {}
reviews_props=(props.get("reviews") or {}).get("properties") or {}
tone_instructions_in_schema="tone_instructions" in (reviews_props or {})
print("CONFIG_KEYS=", sorted(cfg.keys()))
print("SCHEMA_HAS_reviews_tone_instructions=", tone_instructions_in_schema)
if errors:
print(f"VALIDATION=FAIL errors={len(errors)}")
for e in errors[:50]:
path=".".join(str(p) for p in e.path) if e.path else "(root)"
print(f"- path={path} validator={e.validator} message={e.message}")
else:
print("VALIDATION=OK")
PYRepository: GoHyperrr/commerce
Length of output: 213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
SCHEMA_PATH="/tmp/coderabbit-schema.json"
echo "== tone_instructions occurrences =="
grep -R --line-number -o '"tone_instructions"' "$SCHEMA_PATH" | head -n 20 || true
echo "== tone_instruction occurrences =="
grep -R --line-number -o '"tone_instruction"' "$SCHEMA_PATH" | head -n 20 || true
echo
echo "== reviews object shape (best-effort) =="
python3 - <<'PY'
import json
p="/tmp/coderabbit-schema.json"
with open(p,"r",encoding="utf-8") as f:
schema=json.load(f)
props=schema.get("properties",{})
reviews=props.get("reviews",{})
print("schema.properties.reviews keys:", sorted(list(reviews.keys())))
print("reviews.properties keys (if present):", sorted(list((reviews.get("properties") or {}).keys())))
# Also check recursively for any property names containing 'tone'
def find_keys(obj, needle):
out=set()
if isinstance(obj, dict):
for k,v in obj.items():
if needle in k:
out.add(k)
out |= find_keys(v, needle)
elif isinstance(obj, list):
for it in obj:
out |= find_keys(it, needle)
return out
tone_keys=find_keys(schema,"tone_")
print("keys containing 'tone_':", sorted(tone_keys))
PYRepository: GoHyperrr/commerce
Length of output: 1185
Schema URL accepts the config (docs link is 404)
- The
yaml-language-serverschema athttps://coderabbit.ai/integrations/schema.v2.jsonis reachable, returns valid JSON, and.coderabbit.yamlvalidates against it. reviews.tone_instructionsis recognized by the schema (no unrecognized-property issue).https://docs.coderabbit.ai/spec/configurationcurrently returns 404; update the reference URL if you want it to work.
🤖 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 @.coderabbit.yaml at line 3, The schema URL in .coderabbit.yaml currently
points to https://coderabbit.ai/integrations/schema.v2.json (which is valid for
validation) but the human-facing docs link referenced in your config points to a
404 at https://docs.coderabbit.ai/spec/configuration; update that documentation
URL to the correct, reachable docs page (or remove the dead docs reference) so
users can follow the spec; check any occurrences of reviews.tone_instructions in
the file remain valid per the schema after updating the URL.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yml:
- Around line 15-22: Update both checkout steps to pin the actions/checkout
action to a specific commit SHA instead of the floating `@v4` tag and disable
credential persistence by setting persist-credentials: false; specifically
modify the first "uses: actions/checkout@v4" and the second "uses:
actions/checkout@v4" (the step that includes repository/ref/path) to use a fixed
commit SHA (e.g., the provided 11bd71901bbe5b1630ceea73d27597364c9af683 after
you verify it) and add persist-credentials: false under their step inputs so the
GitHub token is not left available to later steps or artifacts.
- Around line 17-22: The "Checkout mdk" step currently checks out the branch tip
and uses actions/checkout@v4 without hard pinning or disabling credentials;
update that step to check out the go.mod-pinned commit SHA
(3d2ab0d97ca9a28fd984dc4a29f6bb611cb62661) by setting the ref to that exact
commit, pin the actions/checkout action to a fixed commit SHA (replace
actions/checkout@v4 with the action's repository@<commit-sha>) and add
persist-credentials: false under the with: block for the "Checkout mdk" step
(the step named "Checkout mdk" and its uses: actions/checkout and with:
repository/ref/path settings).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 84fec220-3633-4f7f-8002-8b07bc370eea
📒 Files selected for processing (1)
.github/workflows/ci.yml
Comment on lines
+15
to
+22
| - name: Checkout commerce | ||
| uses: actions/checkout@v4 | ||
| - name: Checkout mdk | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: GoHyperrr/mdk | ||
| ref: ${{ github.head_ref || github.ref_name }} | ||
| path: ../mdk |
There was a problem hiding this comment.
Harden checkout steps against supply-chain attacks and credential leakage.
Both checkout steps have security posture gaps:
-
Unpinned actions: Using
actions/checkout@v4allows the v4 tag to be mutated, enabling potential supply-chain attacks. Pin to a specific commit SHA. -
Credential persistence: The default
persist-credentials: truecan leak the GitHub token through artifacts or subsequent steps. Set tofalseto minimize exposure.
🔒 Proposed fix: Pin actions and disable credential persistence
- name: Checkout commerce
- uses: actions/checkout@v4
+ uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
+ with:
+ persist-credentials: false
- name: Checkout mdk
- uses: actions/checkout@v4
+ uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2
with:
repository: GoHyperrr/mdk
ref: ${{ github.head_ref || github.ref_name }}
path: ../mdk
+ persist-credentials: falseNote: The SHA 11bd71901bbe5b1630ceea73d27597364c9af683 corresponds to actions/checkout@v4.2.2. Verify the latest stable release before applying.
📝 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
| - name: Checkout commerce | |
| uses: actions/checkout@v4 | |
| - name: Checkout mdk | |
| uses: actions/checkout@v4 | |
| with: | |
| repository: GoHyperrr/mdk | |
| ref: ${{ github.head_ref || github.ref_name }} | |
| path: ../mdk | |
| - name: Checkout commerce | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| persist-credentials: false | |
| - name: Checkout mdk | |
| uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2 | |
| with: | |
| repository: GoHyperrr/mdk | |
| ref: ${{ github.head_ref || github.ref_name }} | |
| path: ../mdk | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.25.2)
[warning] 15-16: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 17-22: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 16-16: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
[error] 18-18: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ci.yml around lines 15 - 22, Update both checkout steps to
pin the actions/checkout action to a specific commit SHA instead of the floating
`@v4` tag and disable credential persistence by setting persist-credentials:
false; specifically modify the first "uses: actions/checkout@v4" and the second
"uses: actions/checkout@v4" (the step that includes repository/ref/path) to use
a fixed commit SHA (e.g., the provided 11bd71901bbe5b1630ceea73d27597364c9af683
after you verify it) and add persist-credentials: false under their step inputs
so the GitHub token is not left available to later steps or artifacts.
Source: Linters/SAST tools
Comment on lines
+17
to
+22
| - name: Checkout mdk | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| repository: GoHyperrr/mdk | ||
| ref: ${{ github.head_ref || github.ref_name }} | ||
| path: ../mdk |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if mdk repository has the feature/mdk-actor-interface branch
gh api repos/GoHyperrr/mdk/git/ref/heads/feature/mdk-actor-interface \
--jq '.ref' 2>&1 || echo "Branch does not exist in mdk repository"Repository: GoHyperrr/commerce
Length of output: 101
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== .github/workflows/ci.yml (checkout-related) ==="
nl -ba .github/workflows/ci.yml | sed -n '1,120p'
echo
echo "=== go.mod (mdk-related) ==="
if [ -f go.mod ]; then
rg -n --hidden --no-messages -S '(^require\b|^\s*require\b|replace\b).*mdk|\bGoHyperrr/mdk\b' go.mod || true
mdk_line="$(rg -n --hidden --no-messages -S '\bGoHyperrr/mdk\b' go.mod | head -n1 | cut -d: -f1 || true)"
if [ -n "${mdk_line:-}" ]; then
start=$((mdk_line-3)); end=$((mdk_line+5))
nl -ba go.mod | sed -n "${start},${end}p"
fi
fi
echo
echo "=== mdk repo: branch tip vs pinned commit (3d2ab0d97ca9) ==="
# Verify the branch tip commit SHA
gh api repos/GoHyperrr/mdk/git/ref/heads/feature/mdk-actor-interface --jq '.object.sha' 2>&1 | head -n 5 || true
# Verify pinned commit exists in mdk repo
gh api repos/GoHyperrr/mdk/commits/3d2ab0d97ca9 --jq '.sha' 2>&1 | head -n 5 || trueRepository: GoHyperrr/commerce
Length of output: 175
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Print .github/workflows/ci.yml (relevant portion) ==="
if [ -f .github/workflows/ci.yml ]; then
# show first 120 lines with line numbers (without nl)
awk 'NR<=120{printf "%d:%s\n", NR, $0}' .github/workflows/ci.yml
else
echo "File .github/workflows/ci.yml not found"
fi
echo
echo "=== Print go.mod (mdk-related require/replace) ==="
if [ -f go.mod ]; then
rg -n --hidden --no-messages -S '\bGoHyperrr/mdk\b' go.mod || true
# Print a small window around the first match
mdk_line="$(rg -n --hidden --no-messages -S '\bGoHyperrr/mdk\b' go.mod | head -n1 | cut -d: -f1 || true)"
if [ -n "${mdk_line:-}" ]; then
start=$((mdk_line-5)); end=$((mdk_line+10))
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%d:%s\n", NR, $0}' go.mod
fi
echo
echo "=== Extract pinned pseudo-version + commit from go.mod ==="
pinned="$(rg -o --no-filename -S 'GoHyperrr/mdk\s+v[0-9A-Za-z.\-]+' go.mod | head -n1 || true)"
echo "Pinned require match: ${pinned:-<none>}"
# Extract the pseudo-version string after mdK (v0.0.0-...-<commit>)
pseudo="$(echo "${pinned:-}" | sed -E 's/.*\s+(v[0-9A-Za-z.\-]+)$/\1/')"
echo "Pseudo-version: ${pseudo:-<none>}"
# Commit is the part after the last '-' in pseudo-version
commit="$(echo "${pseudo:-}" | awk -F'-' '{print $NF}')"
echo "Commit candidate from pseudo-version: ${commit:-<none>}"
else
echo "go.mod not found"
commit=""
fi
echo
echo "=== mdk repo: branch tip commit for feature/mdk-actor-interface ==="
gh api repos/GoHyperrr/mdk/git/ref/heads/feature/mdk-actor-interface --jq '.object.sha' 2>&1 | head -n 20
echo
echo "=== mdk repo: check whether pinned commit exists (if we got one) ==="
if [ -n "${commit:-}" ] && [[ "${commit}" != "<none>" ]]; then
gh api repos/GoHyperrr/mdk/commits/$commit --jq '.sha' 2>&1 | head -n 20
else
echo "No pinned commit extracted from go.mod to check."
fi
echo
echo "=== Check workflow for persist-credentials setting on checkout steps ==="
if [ -f .github/workflows/ci.yml ]; then
rg -n --hidden --no-messages -S 'persist-credentials' .github/workflows/ci.yml || true
rg -n --hidden --no-messages -S 'uses:\s*actions/checkout@' .github/workflows/ci.yml || true
fiRepository: GoHyperrr/commerce
Length of output: 2246
🌐 Web query:
actions/checkout@v4 persist-credentials default value
💡 Result:
The input persist-credentials for actions/checkout@v4 defaults to true.[1][2] The README states that the auth token (or SSH key) is persisted in the local git config and removed during post-job cleanup; it also notes to set persist-credentials: false to opt out.[2][3] So the default value is: persist-credentials: true.[1][2]
Citations:
- 1: https://github.com/actions/checkout/blob/v4.1.1/action.yml
- 2: https://github.com/actions/checkout/blob/v4/README.md
- 3: https://github.com/actions/checkout/?tab=readme-ov-file
Fix mdk checkout to match the go.mod-pinned commit (and harden checkout credentials/action pinning)
GoHyperrr/mdkis checked out withref: ${{ github.head_ref || github.ref_name }}(lines 17-22). Even though the branch exists, the checked-out commit at the branch tip (f09b2ecbff011850ce221ffb5b499d2a2d510b89) does not match thego.mod-pinned pseudo-version commit (3d2ab0d97ca9a28fd984dc4a29f6bb611cb62661), so CI can compile/test against the wrongmdkrevision.actions/checkout@v4is not pinned to a commit SHA (lines 15-22), andpersist-credentialsis omitted (defaults totrue), increasing credential leakage risk.
🔧 Proposed fix: Checkout the pinned commit SHA
- name: Checkout mdk
uses: actions/checkout@v4
with:
repository: GoHyperrr/mdk
- ref: ${{ github.head_ref || github.ref_name }}
+ ref: 3d2ab0d97ca9
path: ../mdk🧰 Tools
🪛 zizmor (1.25.2)
[warning] 17-22: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[error] 18-18: unpinned action reference (unpinned-uses): action is not pinned to a hash (required by blanket policy)
(unpinned-uses)
🤖 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 @.github/workflows/ci.yml around lines 17 - 22, The "Checkout mdk" step
currently checks out the branch tip and uses actions/checkout@v4 without hard
pinning or disabling credentials; update that step to check out the
go.mod-pinned commit SHA (3d2ab0d97ca9a28fd984dc4a29f6bb611cb62661) by setting
the ref to that exact commit, pin the actions/checkout action to a fixed commit
SHA (replace actions/checkout@v4 with the action's repository@<commit-sha>) and
add persist-credentials: false under the with: block for the "Checkout mdk" step
(the step named "Checkout mdk" and its uses: actions/checkout and with:
repository/ref/path settings).
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/ci.yml (2)
15-16:⚠️ Potential issue | 🟠 MajorSecurity posture gaps remain unaddressed.
The security issues flagged in previous reviews are still present:
actions/checkout@v4is unpinned (should use commit SHA)persist-credentialsdefaults totrue(should be set tofalse)🤖 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 @.github/workflows/ci.yml around lines 15 - 16, The workflow uses an unpinned checkout action and leaves persist-credentials at its default; update the checkout step that currently uses actions/checkout@v4 to reference a specific commit SHA for that action (pin to the exact git sha) and explicitly set persist-credentials: false in the same step to avoid leaking GITHUB_TOKEN to subsequent steps (locate the step using the identifier actions/checkout@v4 and modify its configuration to include the commit SHA and persist-credentials: false).
17-22:⚠️ Potential issue | 🔴 CriticalCritical ref mismatch and security issues remain unaddressed.
The issues flagged in previous reviews are still present:
ref: ${{ github.head_ref || github.ref_name }}checks out the branch tip instead of the go.mod-pinned commit (3d2ab0d97ca9a28fd984dc4a29f6bb611cb62661), causing CI to test against a different mdk revision than what's pinnedactions/checkout@v4is unpinned (should use commit SHA)persist-credentialsdefaults totrue(should be set tofalse)🤖 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 @.github/workflows/ci.yml around lines 17 - 22, The checkout step is insecure and uses the branch tip instead of the go.mod‑pinned commit; update the checkout step (the "Checkout mdk" step that uses actions/checkout) to: pin the actions/checkout action to its full commit SHA instead of `@v4`, set with.ref to the exact commit SHA "3d2ab0d97ca9a28fd984dc4a29f6bb611cb62661" (so CI tests the pinned mdk revision), and add with.persist-credentials: false to avoid leaking repo credentials; keep the existing repository and path fields unchanged.
🤖 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 @.github/workflows/ci.yml:
- Around line 23-24: The "Move mdk to parent directory" step uses "mv mdk
../mdk" with no guards; update that step to first check that the source "mdk"
exists and fail with a clear message if not, remove or rename the destination
"../mdk" if it already exists (e.g., rm -rf ../mdk) before performing the move,
perform the move ("mv mdk ../mdk") and then verify success (check that ../mdk
now exists) and fail with an explicit error message if the move failed;
reference the step name "Move mdk to parent directory" and the command "mv mdk
../mdk" when making the changes.
---
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 15-16: The workflow uses an unpinned checkout action and leaves
persist-credentials at its default; update the checkout step that currently uses
actions/checkout@v4 to reference a specific commit SHA for that action (pin to
the exact git sha) and explicitly set persist-credentials: false in the same
step to avoid leaking GITHUB_TOKEN to subsequent steps (locate the step using
the identifier actions/checkout@v4 and modify its configuration to include the
commit SHA and persist-credentials: false).
- Around line 17-22: The checkout step is insecure and uses the branch tip
instead of the go.mod‑pinned commit; update the checkout step (the "Checkout
mdk" step that uses actions/checkout) to: pin the actions/checkout action to its
full commit SHA instead of `@v4`, set with.ref to the exact commit SHA
"3d2ab0d97ca9a28fd984dc4a29f6bb611cb62661" (so CI tests the pinned mdk
revision), and add with.persist-credentials: false to avoid leaking repo
credentials; keep the existing repository and path fields unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 69896cb9-f3df-476d-91a8-630d77f63771
📒 Files selected for processing (1)
.github/workflows/ci.yml
Comment on lines
+23
to
+24
| - name: Move mdk to parent directory | ||
| run: mv mdk ../mdk |
There was a problem hiding this comment.
Add defensive checks for the move operation.
The mv command has no safeguards:
- If
../mdkalready exists (e.g., from runner caching or a previous step),mvwill fail - If the checkout step failed silently,
mdk/may not exist - The error message won't clearly indicate the root cause
Consider adding an explicit cleanup step before the move:
🛡️ Proposed fix: Add defensive cleanup
+ - name: Clean up any existing mdk in parent
+ run: rm -rf ../mdk
- name: Move mdk to parent directory
run: mv mdk ../mdkAlternatively, verify the move succeeded:
- name: Move mdk to parent directory
- run: mv mdk ../mdk
+ run: |
+ mv mdk ../mdk
+ test -d ../mdk || { echo "Failed to move mdk to parent directory"; exit 1; }🤖 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 @.github/workflows/ci.yml around lines 23 - 24, The "Move mdk to parent
directory" step uses "mv mdk ../mdk" with no guards; update that step to first
check that the source "mdk" exists and fail with a clear message if not, remove
or rename the destination "../mdk" if it already exists (e.g., rm -rf ../mdk)
before performing the move, perform the move ("mv mdk ../mdk") and then verify
success (check that ../mdk now exists) and fail with an explicit error message
if the move failed; reference the step name "Move mdk to parent directory" and
the command "mv mdk ../mdk" when making the changes.
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.
Updates commerce dependencies and configuration for mdk integration.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores