diff --git a/plans/pr-review-issues-prioritized.md b/plans/pr-review-issues-prioritized.md new file mode 100644 index 0000000..eac481a --- /dev/null +++ b/plans/pr-review-issues-prioritized.md @@ -0,0 +1,243 @@ +# PR Review Issues - Prioritized Action Plan + +**PR**: refactor: move registry item installation to the core +**Date**: 2026-02-07 +**Reviewer**: copilot-pull-request-reviewer[bot] + +## Summary + +This document collects and prioritizes all issues identified in the code review for the registry refactor PR. Issues are ranked by severity and impact on functionality, security, and maintainability. + +--- + +## 🔴 Critical Priority (Must Fix) + +### 1. Path Traversal Vulnerability in Registry Fetcher + +**Files**: +- `packages/core/src/features/registry/api.ts:6-8` +- `packages/core/src/features/registry/api.ts:12-16` + +**Issue**: The core registry fetcher interpolates `path` directly into the URL without validation/normalization. This allows `../` (and encoded variants) to escape the registry prefix and fetch arbitrary paths under the same origin. + +**Impact**: Security vulnerability - potential unauthorized access to arbitrary paths + +**Action Required**: Apply the same `validateRegistryPath()` guard that the CLI version uses: +- Remove leading slashes +- Reject traversal segments (`..`) +- Reject encoded traversal variants + +**Priority Justification**: Security vulnerabilities must be addressed before merge. + +--- + +### 2. TypeScript Compilation Failure - Missing BranchConfig Import + +**File**: `packages/core/src/index.ts:544-548` + +**Issue**: `BranchConfig` is referenced in the `installRegistryItemToXano` signature but is not imported in this file. + +**Impact**: TypeScript compilation will fail, blocking builds + +**Action Required**: Import `BranchConfig` alongside `InstanceConfig`/`WorkspaceConfig` from `@repo/types` + +**Priority Justification**: Compilation errors block all development and deployment. + +--- + +### 3. Null Dereference in InstallParams.instanceConfig + +**Files**: +- `packages/core/src/features/registry/install-to-xano.ts:5-11` +- `packages/core/src/features/registry/install-to-xano.ts:75` + +**Issue**: `InstallParams.instanceConfig` is typed as `InstanceConfig | null`, but the implementation unconditionally dereferences `instanceConfig.name`. + +**Impact**: Runtime error when instanceConfig is null + +**Action Required**: Either: +- Make `instanceConfig` non-nullable throughout the module (preferred if it must exist to install) +- OR handle the null case before calling `loadToken` + +**Priority Justification**: Runtime crashes are critical bugs. + +--- + +## 🟠 High Priority (Should Fix) + +### 4. Unstable Sorting in sortByTypePriority + +**File**: `packages/core/src/features/registry/general.ts:28-29` + +**Issue**: `typePriority[a.type]` / `typePriority[b.type]` can be `undefined` for unexpected/unknown item types, which makes the comparator return `NaN` and results in unstable/no-op sorting. + +**Impact**: Non-deterministic sorting behavior, test failures + +**Action Required**: Use numeric fallback for both priorities: +```typescript +const aPriority = typePriority[a.type] ?? 99; +const bPriority = typePriority[b.type] ?? 99; +``` + +**Priority Justification**: Breaks expected behavior and existing tests. + +--- + +### 5. Install Flow Using Summary Instead of Full Registry Item + +**Files**: +- `packages/cli/src/commands/registry/implementation/registry.ts:36` +- `packages/cli/src/commands/registry/implementation/registry.ts:45` +- `packages/cli/src/commands/registry/implementation/registry.ts:71-75` + +**Issue**: The install flow pulls the "item" from `index.json` and passes it directly to `installRegistryItemToXano`. If `index.json` items are summaries (commonly `{ name, description, ... }`) and not full install manifests (with `files`/`content`), installs will silently no-op or fail. + +**Impact**: Registry installs may fail silently or not work at all + +**Action Required**: Call `core.getRegistryItem(componentName, registryUrl)` to ensure you're installing the full registry item definition (not just the summary from index.json). + +**Priority Justification**: Core feature functionality broken. + +--- + +### 6. results.skipped Never Populated (Regression) + +**Files**: +- `packages/core/src/features/registry/install-to-xano.ts:64` +- `packages/core/src/features/registry/install-to-xano.ts:124-129` + +**Issue**: `results.skipped` is never populated, so "already exists" / idempotent install cases will be reported as failures (regression vs prior CLI behavior). + +**Impact**: Poor UX - legitimate skipped cases reported as failures + +**Action Required**: Parse the error JSON body from Xano (when available) and map known "already exists/duplicate" errors into `skipped` (keep `failed` for real errors). + +**Priority Justification**: User-facing regression in error reporting. + +--- + +### 7. Browser Storage readdir() Double-Slash Bug + +**File**: `packages/browser-consumer/src/browser-config-storage.ts:134-138` + +**Issue**: `readdir()` appends `'/'` unconditionally. If the caller already passes `'dir/'`, the prefix becomes `'dir//'` and `listFiles()` won't match keys like `'dir/file1.txt'`. + +**Impact**: Directory listing will fail in browser storage + +**Action Required**: Normalize by trimming trailing slashes before appending one. + +**Priority Justification**: Breaks browser-consumer functionality. + +--- + +### 8. Browser Storage readFile() Binary Data Handling + +**File**: `packages/browser-consumer/src/browser-config-storage.ts:145-156` + +**Issue**: `readFile()` will almost always return a string for binary data because `TextDecoder().decode()` typically does not throw on arbitrary bytes—so the "fallback to Uint8Array" path won't run. + +**Impact**: Binary file handling broken in browser storage + +**Action Required**: If `ConfigStorage.readFile` is meant to behave like the Node implementation (Buffer/Uint8Array), return `Uint8Array` consistently (or store metadata to distinguish binary vs text). + +**Priority Justification**: Data corruption for binary files. + +--- + +## 🟡 Medium Priority (Nice to Have) + +### 9. Duplicate Type Definitions + +**File**: `packages/core/src/features/testing/index.ts:7-43` + +**Issue**: This file (and `packages/core/src/implementations/run-tests.ts`) defines `TestConfigEntry/TestResult/TestGroupResult/AssertContext` inline, but this PR also adds exported equivalents to `@repo/types`. + +**Impact**: Type drift risk, maintenance burden + +**Action Required**: Switch core to import the canonical types from `@repo/types` and remove the inline interfaces. + +**Priority Justification**: Technical debt that increases maintenance cost. + +--- + +### 10. runTest() Process Control Issues + +**File**: `packages/cli/src/commands/test/implementation/test.ts:283-301` + +**Issue**: `runTest()` both returns an exit code and calls `process.exit(1)` in CI mode. This makes the function hard to reuse programmatically and complicates unit testing. + +**Impact**: Reduced testability and reusability + +**Action Required**: Prefer returning the exit code and letting the commander action decide (e.g., set `process.exitCode = 1` or call `process.exit(code)` only at the CLI boundary). + +**Priority Justification**: Code quality improvement. + +--- + +### 11. Missing DBSchema Extension for CalyDBSchema + +**Files**: +- `packages/browser-consumer/src/indexeddb-utils.ts:1` +- `packages/browser-consumer/src/indexeddb-utils.ts:13-17` + +**Issue**: For `idb`, the schema generic is typically constrained to `DBSchema`. If `CalyDBSchema` doesn't extend `DBSchema`, TypeScript may reject `openDB(...)` or lose type safety. + +**Impact**: Potential type safety loss + +**Action Required**: Import `DBSchema` from `idb` and declare `interface CalyDBSchema extends DBSchema { ... }`. + +**Priority Justification**: Type safety improvement. + +--- + +### 12. Missing Test Config Example Field + +**File**: `packages/cli/examples/config.js:13` + +**Issue**: The test runner expects each parameter to include an `in` field (`'path' | 'query' | 'header' | 'cookie'`), and the JSON schema also requires it. This example config omits `in`. + +**Impact**: Example config will fail validation + +**Action Required**: Update example entries to include `in: 'query'` (and similarly ensure path params include `in: 'path'`). + +**Priority Justification**: Documentation accuracy. + +--- + +## 🟢 Low Priority (Future Work) + +### 13. Missing Unit Tests for installRegistryItemToXano + +**File**: `packages/core/src/features/registry/install-to-xano.ts:57-62` + +**Issue**: `installRegistryItemToXano` introduces new core behavior (hybrid inline/file content, meta API posting, query apiGroupId requirements, and install result shaping) but has no unit tests. + +**Impact**: Reduced confidence in behavior, harder to catch regressions + +**Action Required**: Add tests for: +1. Inline `content` install +2. Path-based fetch install +3. Query install error when apiGroupId missing +4. Error body parsing/skipped behavior + +**Priority Justification**: Test coverage improvement for future maintenance. + +--- + +## Recommended Fix Order + +1. **Immediate (Block merge)**: Issues #1, #2, #3 - Security and compilation blockers +2. **Before merge**: Issues #4, #5, #6, #7, #8 - Core functionality and correctness +3. **Post-merge/Next sprint**: Issues #9, #10, #11, #12 - Code quality and documentation +4. **Backlog**: Issue #13 - Test coverage improvement + +--- + +## Notes + +- Total issues identified: 13 +- Critical/High priority: 8 issues +- Medium priority: 4 issues +- Low priority: 1 issue + +All issues should be tracked and addressed according to their priority level. Critical and high-priority issues must be resolved before merging this PR.