agents icon indicating copy to clipboard operation
agents copied to clipboard

make `pnpm dev` run `build` in workspaces I depend on first, rename `turbo.json` to `turbo.jsonc`

Open dimaMachina opened this issue 1 month ago • 3 comments

I've run into issue where pnpm dev fails due missing build command in @inkeep/agents-manage-mcp

image

dimaMachina avatar Nov 25 '25 10:11 dimaMachina

⚠️ No Changeset found

Latest commit: 0c49b71db98ae85369dae9a6d405aa95a60bc0b4

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Nov 25 '25 10:11 changeset-bot[bot]

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Preview Comments Updated (UTC)
agents-docs Ready Ready Preview Comment Dec 1, 2025 9:23pm
agents-manage-api Ready Ready Preview Comment Dec 1, 2025 9:23pm
agents-manage-ui Ready Ready Preview Comment Dec 1, 2025 9:23pm
agents-run-api Ready Ready Preview Comment Dec 1, 2025 9:23pm
agents-test Ready Ready Preview Comment Dec 1, 2025 9:23pm

vercel[bot] avatar Nov 25 '25 10:11 vercel[bot]

Claude finished @dimaMachina's task —— View job


PR Review: Build Dependencies and Turbo Configuration

I've completed a comprehensive review of this PR that addresses build dependency issues and Turbo configuration updates. Here's my detailed analysis:

🎯 Core Fix Analysis

✅ Primary Issue Resolution The addition of "^build" dependency to the dev task (turbo.jsonc:35-38) correctly solves the reported issue where pnpm dev failed due to missing built dependencies. This ensures workspace dependencies are built before starting development servers.

🔧 Technical Changes Review

1. Turbo Configuration (turbo.json → turbo.jsonc)

  • Good: Rename to .jsonc enables JSON with comments, improving maintainability
  • Good: Added explicit comment explaining the ^build dependency logic
  • Good: Preserves all existing task configurations and dependencies

2. Package Configuration Updates

  • Good: Clean line changes in package.json (lines 37-38) adding esm directory reference for @inkeep/agents-manage-mcp
  • Note: This aligns with the MCP package's export structure using esm/ output directory

3. Dockerfile Updates

  • Good: Consistent update across all Dockerfiles to reference turbo.jsonc instead of turbo.json
  • Concern: Missing verification that Docker builds work with the new configuration

📋 Code Quality Assessment

API Changes (agents-manage-api/src/middleware/require-permission.ts)

  • Good: Clean import addition for proper error handling
  • Maintainability: The permission middleware logic is well-structured and secure

UI Changes (agents-manage-ui/)

  • Good: Proper error handling and logging in copilot token management
  • Good: Consistent API configuration patterns
  • Good: Clean unused import removal

SDK & Core Updates

  • Good: Test coverage additions in agents-core for comprehensive agent definition scenarios
  • Good: Tool interface improvements with proper context setting

🏗️ Architectural Considerations

Build Pipeline Impact

  • ✅ Positive: The ^build dependency creates proper build order
  • ✅ Positive: Maintains development server performance by building dependencies once
  • ⚠️ Consider: Impact on incremental builds during development

Workspace Dependencies

  • ✅ Good: Addresses the specific MCP package build requirement
  • ✅ Good: Maintains consistency with existing Turbo patterns
  • ✅ Good: The @inkeep/agents-manage-mcp specific build configuration (lines 97-101) properly handles ESM output

🚨 Potential Issues & Recommendations

Critical

  1. Deployment Verification: Vercel deployment for agents-manage-api shows "Error" status. This needs investigation before merge.

Medium Priority

  1. Build Performance: Consider impact on development workflow - developers may experience longer startup times
  2. Documentation: The README updates look good, but consider adding dev workflow guidance

Low Priority

  1. Consistency: All changes follow existing patterns well

🧪 Testing & Validation

What's Covered

  • ✅ Comprehensive test coverage additions for agent data access
  • ✅ Tool interface and configuration testing
  • ✅ Proper mock setup for complex scenarios

Missing Verification

  • ❓ End-to-end build pipeline testing
  • ❓ Docker build validation with new turbo.jsonc
  • ❓ Performance impact measurement on development startup

🎯 Customer Impact

Positive

  • ✅ Fixes blocking development workflow issue
  • ✅ Improves developer experience with working pnpm dev
  • ✅ Maintains backward compatibility

Risk Assessment

  • ⚠️ Build Time: May increase initial development startup time
  • ⚠️ Deployment: Current Vercel build failure needs resolution

📝 Final Recommendations

Before Merge

  1. Critical: Investigate and resolve the Vercel deployment failure for agents-manage-api
  2. Recommended: Test the complete dev workflow end-to-end with the new configuration
  3. Good Practice: Consider adding a brief note in development documentation about the new build dependency behavior

Overall Assessment

This PR addresses the core issue effectively with a clean, maintainable solution. The architectural approach is sound, and the code quality is high. The primary concern is the deployment failure that needs immediate attention.

The change is architecturally sound and follows best practices for monorepo build orchestration. Once the deployment issue is resolved, this should be safe to merge.


claude[bot] avatar Nov 25 '25 10:11 claude[bot]