chore(npm): change npm to and fix workflows - #2
Conversation
kfirstri
commented
Jan 5, 2026
- Move from yarn to npm
- Add github workflows
Pull Request Review: Migration from Yarn to npmI've reviewed this PR that migrates the project from Yarn to npm and adds GitHub workflows. Here's my feedback: ✅ Positive Changes
🔴 Critical Issues1. Security: Missing NPM_TOKEN Secret Configuration (manual-publish.yml:84)The manual publish workflow references - name: Publish to NPM
working-directory: packages/cli
run: npm publish --tag ${{ github.event.inputs.npm_tag }}Fix Required: Add 2. Security: Missing NPM_TOKEN in Preview Publish (preview-publish.yml:106)Same issue - the preview publish workflow needs authentication: - name: Publish preview package
working-directory: packages/cli
run: npm publish --tag previewFix Required: Add authentication before publishing. 3. Git Configuration Security Risk (manual-publish.yml:96-97)git config --local user.email "action@github.com"
git config --local user.name "GitHub Action"Issue: Using generic email/name for commits. Consider using a project-specific bot account or the GitHub Actions bot. 4. Dangerous Git Push (manual-publish.yml:101)git push origin HEAD:${{ github.ref }}Issue: Pushing to
|
PR Review: Migration from Yarn to npm and GitHub WorkflowsThank you for this comprehensive PR. Here is my detailed review: Positive Aspects
Critical Issues (Must Fix)1. Missing NPM_TOKEN in workflows (.github/workflows/manual-publish.yml:85, preview-publish.yml:108)
2. Deprecated GitHub Action (manual-publish.yml:106)
3. Preview workflow logic error (preview-publish.yml:51)
High Priority Issues
Medium Priority Issues
SummaryMust fix before merge:
Should fix:
SecurityGood: Pinned action versions, minimal permissions Critical issues 1 and 3 will prevent workflows from working. Let me know if you need help fixing these! |
Pull Request Review: Yarn to NPM MigrationThis PR successfully migrates from Yarn to npm and adds CI/CD workflows. Overall solid work with some critical issues that need attention. Critical Issues1. Missing Build and Type Check Steps Recommendation: Add a CI workflow with build and type-check steps, or enhance the lint workflow. 2. Preview Publish Logic Flaw Recommendation: Change to package_name=base44-preview 3. Missing NPM Authentication Recommendation: Add NODE_AUTH_TOKEN using secrets.NPM_TOKEN 4. Deprecated GitHub Action Recommendation: Use softprops/action-gh-release or gh release create Medium Priority Issues
Strengths
Security Notes
RecommendationStatus: Changes Requested Fix the critical issues (especially preview publish logic and missing build steps) before merge. Once addressed, this will be a great improvement to the project infrastructure. Great work on the comprehensive workflow setup! |
PR Review: Migration from Yarn to npm and Workflow UpdatesSummaryThis PR migrates the project from Yarn to npm and adds comprehensive GitHub Actions workflows for linting, manual publishing, and preview package publishing. The migration appears complete, but there are several security concerns and potential issues that should be addressed. Critical Issues1. Security: Missing NPM_TOKEN in Publish WorkflowsSeverity: High Both Location: Fix Required: Add 2. Security: Deprecated actions/create-releaseSeverity: Medium Location: The Major Issues3. Workflow: Missing Build/Test Steps in Lint WorkflowSeverity: Medium The original Recommendation: Restore build and type-check steps to the CI workflow. 4. Workflow: Race Condition in manual-publish.ymlSeverity: Medium The workflow commits version changes AFTER publishing to npm. If the publish succeeds but git operations fail, the git history will not reflect the published version. Recommended order: Commit → Push → Publish (not Publish → Commit → Push) 5. Workflow: Missing npm Authentication VerificationSeverity: Medium Add a verification step to ensure npm authentication is configured before publishing. Minor Issues6. Inconsistent Dependency InstallationLocation: Uses 7. ESLint Configuration: Limited RulesThe new ESLint config only enforces one rule. Consider adding TypeScript-specific and code quality rules. Positive Aspects✅ Comprehensive safety checks in preview-publish workflow Action Items (Priority Order)
ConclusionThis is a solid migration with well-designed publishing workflows. However, critical security issues (missing NPM_TOKEN, deprecated actions) and missing CI validation must be addressed before merging. Recommendation: Request changes to address blocking and high-priority items. |
- Rename loop variable `i` to `integration` (review #2) - Use typed destructuring instead of Record<string, unknown> (review #3) - Extract reusable formatYaml() utility with stripEmpty option (review #4) - Use lodash isEmpty for null/empty stripping in YAML replacer - Add formatYaml unit tests - Update test assertions for integrationType key rename Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Rename loop variable `i` to `integration` (review #2) - Use typed destructuring instead of Record<string, unknown> (review #3) - Extract reusable formatYaml() utility with stripEmpty option (review #4) - Use lodash isEmpty for null/empty stripping in YAML replacer - Add formatYaml unit tests - Update test assertions for integrationType key rename Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
* feat(connectors): add `connectors list-available` command Add a new CLI command to discover all available integration types from the Base44 catalog endpoint (GET /external-auth/available-integrations). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review — remove unused type, add min(1) to integration_type Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: align schema with apper commit 7556a23 - Wrapper key: `available_integrations` → `integrations` - Remove `notes` and `usage_guide` fields (dropped from backend) - Extract long template literal for readability Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: output full JSON for list-available Print the complete integration data (including connection_config_fields) as JSON to stdout so LLMs and scripts can consume all fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * feat: YAML-formatted output for list-available - Add yaml dep for pretty-printing integration details - Display each integration as YAML (type, description, config fields) - Keep connection_config_fields in snake_case (no transform) - Remove temp fake fields Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: restore camelCase transform for connection config fields Keep consistent with codebase convention — convert snake_case to camelCase at the Zod boundary for all fields. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: add .claude/worktrees/ to gitignore Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: address review findings for list-available - Use camelCase key `connectionConfigFields` in YAML output - Strip null/undefined values from config fields before display - Add comment explaining z.string() choice over IntegrationTypeSchema - Add test for malformed API response (Zod validation) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * refactor: extract formatYaml helper, address review findings - Rename loop variable `i` to `integration` (review #2) - Use typed destructuring instead of Record<string, unknown> (review #3) - Extract reusable formatYaml() utility with stripEmpty option (review #4) - Use lodash isEmpty for null/empty stripping in YAML replacer - Add formatYaml unit tests - Update test assertions for integrationType key rename Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: update bun.lock for yaml dependency Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: use main's root package.json for monorepo paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * style: fix Biome formatting in yaml tests Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>