Skip to content

Conversation

@dimaMachina
Copy link
Collaborator

No description provided.

@changeset-bot
Copy link

changeset-bot bot commented Jan 6, 2026

🦋 Changeset detected

Latest commit: 2130c29

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 9 packages
Name Type
@inkeep/agents-manage-ui Patch
@inkeep/agents-manage-api Patch
@inkeep/agents-run-api Patch
@inkeep/agents-cli Patch
@inkeep/agents-core Patch
@inkeep/agents-manage-mcp Patch
@inkeep/agents-sdk Patch
@inkeep/ai-sdk-provider Patch
@inkeep/create-agents Patch

Not sure what this means? Click here to learn what changesets are.

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

@vercel
Copy link

vercel bot commented Jan 6, 2026

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

Project Deployment Review Updated (UTC)
agents-manage-api Ready Ready Preview, Comment Jan 6, 2026 11:43pm
agents-manage-ui Ready Ready Preview, Comment Jan 6, 2026 11:43pm
agents-run-api Ready Ready Preview, Comment Jan 6, 2026 11:43pm
1 Skipped Deployment
Project Deployment Review Updated (UTC)
agents-docs Skipped Skipped Jan 6, 2026 11:43pm

@claude
Copy link

claude bot commented Jan 6, 2026

Claude finished @dimaMachina's task —— View job


PR Review: Pino Module Loading Fix

I've analyzed this PR and will provide comprehensive engineering feedback on the fix for the pino module loading issue.

Review Tasks:

  • Understand the problem and solution approach
  • Examine the new script implementation
  • Review package.json changes
  • Assess architectural implications
  • Check for potential side effects and alternatives
  • Provide feedback on code quality and best practices

Issue Analysis

The problem stems from Turbopack's hash suffixing mechanism for server-only packages. When @inkeep/agents-manage-ui is used as a dependency, Turbopack appends hash suffixes to package names (e.g., pino-51ec28aa490c8dec), causing module resolution failures during runtime.

This is a Next.js 16.1.0 + Turbopack specific issue where the build system modifies package names but the runtime package.json doesn't reflect these changes.

Technical Solution Review

The implemented solution uses a prepublishOnly script that dynamically patches the package.json to include the hashed dependencies:

Script Analysis (append-hash-for-server-only-packages.mts)

Strengths:

  • ✅ Addresses the root cause by syncing package.json with Turbopack's naming
  • ✅ Uses prepublishOnly hook to ensure it runs before publishing
  • ✅ Dynamic approach that adapts to any hash changes

Critical Issues:

  1. No Error Handling agents-manage-ui/scripts/append-hash-for-server-only-packages.mts:15-32

    fs.readdir(NODE_MODULES_PATH).then(async (dirs) => {
      // Missing try-catch blocks for all file operations
    • Risk: Silent failures could result in incomplete dependency mappings
    • Impact: Runtime module resolution failures in production
  2. Hard-coded Path Assumptions agents-manage-ui/scripts/append-hash-for-server-only-packages.mts:13

    const NODE_MODULES_PATH = '.next/node_modules';
    • Risk: Script fails if Turbopack changes output structure
    • Impact: Build-time failures during publishing
  3. Dependency Overwriting Risk agents-manage-ui/scripts/append-hash-for-server-only-packages.mts:24-28

    dependencies: {
      ...appPackageJson.dependencies,
      ...deps, // Could overwrite existing dependencies
    },
    • Risk: Hash-suffixed dependencies could conflict with existing ones
    • Impact: Unpredictable module resolution behavior

Architectural Concerns

1. Workaround vs. Root Cause Fix

This is a band-aid solution that treats symptoms rather than the underlying Turbopack issue. Consider:

  • Filing issue with Next.js/Turbopack team
  • Exploring Turbopack configuration options (serverExternalPackages, transpilePackages)
  • Investigating if pino can be moved to client-side dependencies

2. Package Publishing Complexity

The prepublishOnly approach adds significant complexity:

  • Script modifies package.json during publish process
  • Increases risk of publishing corrupted packages
  • Makes debugging dependency issues more difficult

3. Maintenance & Debugging Challenges

  • Hard-coded .next/node_modules path won't work for all deployment scenarios
  • Solution is specific to pino but may need to scale to other server-only packages
  • No mechanism to handle hash changes between builds
  • Difficult to troubleshoot when the script fails

Recommended Improvements

High Priority - Error Handling

// Add comprehensive error handling
try {
  const dirs = await fs.readdir(NODE_MODULES_PATH);
  // ... rest of logic
} catch (error) {
  console.error('Failed to read .next/node_modules:', error);
  process.exit(1);
}

Medium Priority - Validation & Safety

// Validate directory structure before processing
if (!await fs.access(NODE_MODULES_PATH).catch(() => false)) {
  console.warn('Skipping hash append - .next/node_modules not found');
  process.exit(0);
}

// Conflict detection for dependencies
const conflicts = Object.keys(deps).filter(key => 
  appPackageJson.dependencies[key] && 
  appPackageJson.dependencies[key] !== deps[key]
);
if (conflicts.length > 0) {
  throw new Error(`Dependency conflicts detected: ${conflicts.join(', ')}`);
}

Low Priority - Alternative Approaches

Consider exploring these alternatives:

  1. Turbopack Configuration: Add pino to transpilePackages in next.config.ts
  2. External Package Declaration: Use Next.js serverExternalPackages config
  3. Runtime Detection: Move dependency resolution to runtime rather than build-time

Security & Best Practices

Node.js Experimental Flag Usage

The script uses --experimental-strip-types which:

  • ✅ Is appropriate for TypeScript execution
  • ⚠️ May not be available in all Node.js versions
  • ⚠️ Could break in future Node.js updates

File System Security

  • ✅ Uses relative paths (good for security)
  • ⚠️ No validation of package.json content structure
  • ⚠️ Overwrites package.json without backup

Impact Assessment

Customer Impact

  • Positive: Fixes module resolution failures in production
  • Risk: Publishing failures could break CI/CD pipeline
  • Mitigation: Add comprehensive error handling and testing

Development Experience

  • Positive: Transparent fix during publishing
  • Risk: Debugging dependency issues becomes more complex
  • Risk: Script failures could block releases

Summary

This PR provides a functional workaround for a legitimate Turbopack issue, but introduces significant technical debt. The solution lacks error handling and makes assumptions about file system structure that may not hold in all environments.

Recommendation:

  1. Accept with conditions - Add comprehensive error handling before merging
  2. Track as technical debt - Create follow-up issues to explore proper Turbopack configuration
  3. Consider alternatives - Investigate transpilePackages configuration as a cleaner solution

The fix addresses an immediate problem but should be treated as a temporary solution while pursuing a more robust architectural approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants