feat: multi-product registry — ProductRegistry schema, parser, and /api/products routes#11
Merged
Merged
Conversation
ProductRegistrySchema validates .helm/products.yaml — a list of knowledge repo paths (relative to a base dir, or absolute). loadProductRegistry() resolves each path, loads its .helm/product.yaml, and returns Product[]. parseProductRegistryYaml() validates raw YAML without I/O. Backward-compat: no changes to existing exports. The registry is an opt-in layer on top of the existing single-product flow. 12 tests covering: single/multi entry, dot-path resolution, missing registry file (ENOENT), missing product.yaml (ENOENT), malformed product.yaml, invalid registry YAML, empty products array. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…outes getProductRegistry() reads $HELM_KNOWLEDGE_REPO_PATH/.helm/products.yaml and resolves each listed path (relative to dirname(HELM_KNOWLEDGE_REPO_PATH)) to a Product. ENOENT on products.yaml → falls back to [getProductConfig()] for single-product backward compat. Single-flight + cache. New endpoints: GET /api/products → all registered Products GET /api/products/:slug → one Product by slug (404 if unknown) GET /api/products/:slug/items → items filtered by productSlug (404 if unknown product) Existing /api/product and /api/items are unchanged — 15 existing tests pass without modification. _resetForTests() extended with registry state. 11 new route tests covering happy paths, cross-product isolation, empty results, 404, and 500 paths. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds product-registry schemas and parser/loader, exposes a cached getProductRegistry() in services with single-flight and legacy fallback, implements products API routes with tests, re-exports registry utilities, and mounts the new router in the app. ChangesMulti-product registry and API routes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Route as productsRouter
participant Service as getProductRegistry
participant Cache as Cached Registry
participant Loader as loadProductRegistry
participant Disk as Filesystem
Client->>Route: GET /products or /products/:slug or /products/:slug/items
Route->>Service: getProductRegistry()
Service->>Cache: check cache
alt registry cached
Cache-->>Service: return cached Promise
else cache miss
Service->>Loader: loadProductRegistry(registryPath, baseDir)
Loader->>Disk: read products.yaml
Loader->>Disk: resolve entry paths and read .helm/product.yaml
Loader-->>Service: Product[]
end
Service-->>Route: Product[]
Route->>Route: validate slug / lookup / filter items
alt match found
Route-->>Client: HTTP 200 + JSON
else not found
Route-->>Client: HTTP 404
end
alt service error
Route-->>Client: HTTP 500 + error message
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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: 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 `@apps/api/src/routes/products.ts`:
- Around line 20-21: The route handler using
productsRouter.get('/products/:slug') reads c.req.param('slug') without
validation; create a strict Zod params schema (e.g. const paramsSchema =
z.object({ slug: z.string().min(1).regex(/^[a-z0-9-]+$/) }).strict()) and
validate the incoming params before using the slug (use paramsSchema.parse or
safeParse on { slug: c.req.param('slug') }); if validation fails return a 400
response and do not proceed to lookup/filtering. Apply the same pattern to the
other route reading params at the other occurrence (lines 37-38) so all external
route params are validated with Zod.strict().
In `@apps/api/src/services/index.ts`:
- Around line 121-122: getProductRegistry currently returns the cached
_productRegistry or the pending _productRegistryPromise directly, exposing
mutable internal state; change getProductRegistry to return defensive copies
instead: when returning _productRegistry return a shallow-copied array (e.g.
[..._productRegistry]) and ensure each product object is cloned (e.g.
_productRegistry.map(p => ({...p})); when resolving or returning
_productRegistryPromise ensure the promised value is similarly cloned before
resolving/returning so callers never receive references to the internal
_productRegistry; apply the same defensive-copy approach to the related
getter/return logic around the code at lines 143-149 to avoid leaking mutable
cache objects.
🪄 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
Run ID: a7a2d300-81bd-4546-9e79-531be204d3b8
📒 Files selected for processing (8)
apps/api/src/app.tsapps/api/src/routes/products.test.tsapps/api/src/routes/products.tsapps/api/src/services/index.tspackages/shared/src/config/index.tspackages/shared/src/config/product-registry-parser.test.tspackages/shared/src/config/product-registry-parser.tspackages/shared/src/config/product-registry-schema.ts
…pies
1. products.ts: validate slug route param against /^[a-z0-9-]+$/ via
SlugParamsSchema before lookup — returns 400 on invalid format.
Adds 2 new tests (400 for :slug and :slug/items).
2. services/index.ts: getProductRegistry() now returns spread copies
{...p} instead of direct cache references to prevent callers from
mutating singleton state.
Addresses CodeRabbit review comments on PR #11.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
apps/api/src/services/index.ts (1)
121-122:⚠️ Potential issue | 🟠 Major | ⚡ Quick winShallow cloning still allows nested cache mutation.
{ ...p }only clones the top level. Nested objects inProductremain shared, so callers can still mutate singleton-backed state through nested fields.As per coding guidelines, "
**/*.{ts,tsx}: Never return mutable internal state directly; spread-copy arrays/objects before returning".Also applies to: 143-145, 148-149
🤖 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 `@apps/api/src/services/index.ts` around lines 121 - 122, The current returns clone only the top-level with ({ ...p }) allowing nested objects in the Product entries to be mutated; replace the shallow spread with a true deep-clone when returning cached products (e.g., use structuredClone(product) or a project deepClone utility) so callers receive fully independent copies; update both the synchronous branch that returns _productRegistry (map over _productRegistry and return structuredClone(p) for each p) and the promise branch that returns _productRegistryPromise (chain .then(results => results.map(r => structuredClone(r)))) and apply the same change to the other similar return sites referenced (the occurrences around lines 143-145 and 148-149).
🤖 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 `@apps/api/src/services/index.ts`:
- Around line 132-139: The ENOENT fallback is too broad; when
loadProductRegistry(registryFilePath, baseDir) throws ENOENT we should only
treat it as "registry file missing" if the missing path is the registryFilePath
(products.yaml). Update the catch to detect err.code === 'ENOENT' && (err.path
=== registryFilePath || err.message?.includes(registryFilePath)) and only then
set products = [await getProductConfig()]; otherwise rethrow the error so
missing referenced product files or other ENOENTs aren’t silently converted to
single-product mode.
---
Duplicate comments:
In `@apps/api/src/services/index.ts`:
- Around line 121-122: The current returns clone only the top-level with ({ ...p
}) allowing nested objects in the Product entries to be mutated; replace the
shallow spread with a true deep-clone when returning cached products (e.g., use
structuredClone(product) or a project deepClone utility) so callers receive
fully independent copies; update both the synchronous branch that returns
_productRegistry (map over _productRegistry and return structuredClone(p) for
each p) and the promise branch that returns _productRegistryPromise (chain
.then(results => results.map(r => structuredClone(r)))) and apply the same
change to the other similar return sites referenced (the occurrences around
lines 143-145 and 148-149).
🪄 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
Run ID: dfb0e006-acfc-4f78-94b3-032ed4e852e9
📒 Files selected for processing (3)
apps/api/src/routes/products.test.tsapps/api/src/routes/products.tsapps/api/src/services/index.ts
Owner
Author
|
@CodeRabbit are we clean? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me check the current state of all review comments on this PR. 🏁 Script executed: Length of output: 37070
What's resolved ✅: slug validation with Zod (commit So there are two actionable items left before merging. Want me to open issues or help plan those fixes? |
…ProductRegistry Previously any ENOENT from loadProductRegistry triggered the single-product fallback, silently hiding broken registry entries (e.g., a referenced product.yaml that is missing). Now the fallback only fires when the error path matches registryFilePath — any other ENOENT propagates so the operator can diagnose the broken entry. Addresses CodeRabbit review comment on PR #11. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… structuredClone
{ ...p } only copies top-level fields; nested objects like workflow,
issue_tracker, and code_repos remained shared references to the singleton
cache. Replaced with structuredClone(p) at all four return sites so
callers cannot mutate cached state through nested properties.
Addresses CodeRabbit review comment on PR #11.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 `@apps/api/src/services/index.ts`:
- Line 122: The cached promise _productRegistryPromise is returned directly
causing concurrent callers to share the same mutable array; change the
early-return to wrap the cached promise so callers receive a defensive copy,
e.g. return _productRegistryPromise.then(products => [...products]) instead of
returning _productRegistryPromise directly, and do the same wherever
_productRegistryPromise is returned so every caller gets a shallow-copied array
(reference symbols: _productRegistryPromise and the function that returns the
registry).
🪄 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
Run ID: f0c42a82-cbca-4df6-a075-7b7ed307e484
📒 Files selected for processing (1)
apps/api/src/services/index.ts
…ry promise Concurrent callers that arrived during initialization all awaited the same _productRegistryPromise, receiving the identical resolved array reference. Wrapping the in-flight promise with .then(products => products.map(structuredClone)) ensures every caller gets its own independent copy, consistent with the cache-hit and post-init paths. Addresses CodeRabbit review comment on PR #11. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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 `@apps/api/src/services/index.ts`:
- Around line 127-131: The code reads HELM_KNOWLEDGE_REPO_PATH into
knowledgePath and immediately uses it to build registryFilePath and baseDir
without validating the value; before constructing paths (before
join(dirname...)), validate knowledgePath against your whitelist regex (e.g.,
EXTERNAL_ID_REGEX or a path-safe regex) and reject or throw an Error if it
doesn't match; ensure you still trim first, perform the regex test on the
trimmed value, and only then compute registryFilePath and baseDir using
join(dirname(knowledgePath)).
🪄 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
Run ID: 64d287f9-ff51-4a7d-a101-096c18341d7a
📒 Files selected for processing (1)
apps/api/src/services/index.ts
…path whitelist HELM_KNOWLEDGE_REPO_PATH is external input (env var) used directly to construct filesystem paths. Added SAFE_FS_PATH_REGEX whitelist check before path construction to prevent path traversal attacks. Addresses CodeRabbit review comment on PR #11. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
…se.path
ProductConfigError exposes .code='ENOENT' but .path lives on .cause
(the original NodeJS.ErrnoException), not directly on the error object.
The previous check ('path' in err) never matched, so the fallback to
single-product mode never fired when products.yaml was absent —
getProductConfig() wasn't called and ENOENT propagated to the route.
Fix: use instanceof ProductConfigError + err.cause?.path === registryFilePath.
Adds 3 regression tests using a real tmpdir:
- absent products.yaml → falls back to [getProductConfig()]
- present products.yaml → loads correctly
- present but referencing missing product.yaml → error propagates
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ePath itself
dirname(knowledgePath) was one level too high: path "." resolved to the parent
of the knowledge repo instead of the repo itself, causing ENOENT on
{parent}/.helm/product.yaml. Fix: baseDir = knowledgePath so that
- path "." → the knowledge repo itself ✓
- path "../X" → a sibling of the knowledge repo ✓
Remove now-unused dirname import.
Adds 2 regression tests verifying path semantics:
- path "." resolves to knowledgePath
- path "../sibling" resolves to a real sibling tmpdir
Updates existing products.yaml test to use path "." (was using basename workaround).
ENOENT fallback unaffected: registryFilePath = join(knowledgePath, '.helm/products.yaml')
unchanged, so err.cause?.path === registryFilePath still holds.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
|
@CodeRabbit resume |
✅ Actions performedReviews resumed. |
This was referenced May 18, 2026
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
@helm/shared:ProductRegistryZod schema andloadProductRegistry()parser. Reads.helm/products.yaml(list of knowledge repo paths, relative to sibling layout or absolute).parseProductRegistryYaml()validates without I/O.@helm/api:getProductRegistry()singleton — loads all products from the registry, falls back to[getProductConfig()]whenproducts.yamlis absent (backward compat). Single-flight + cache.GET /api/products— all registered ProductsGET /api/products/:slug— one Product (404 if unknown)GET /api/products/:slug/items— items filtered byproductSlug(404 if unknown product)/api/productand/api/itemsunchanged — all 15 existing tests pass without modification.Test plan
pnpm --filter @helm/shared test— 24 tests pass (12 new registry tests)pnpm --filter @helm/api test— 58 tests pass (11 new products route tests, 15 existing items tests unchanged)pnpm --filter @helm/api build— cleanKnown limitations documented in ADR-004 (helm-knowledge PR)
externalIdcollision across products is possible when two products have items with the same ID (filesystem uses flatdata/items/{externalId}.json). Deferred to v1 withdata/items/{productSlug}/{externalId}.jsonnamespacing.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Exports
Tests