Skip to content

Conversation

@NathanFlurry
Copy link
Member

No description provided.

@vercel
Copy link

vercel bot commented Oct 24, 2025

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

3 Skipped Deployments
Project Deployment Preview Comments Updated (UTC)
rivet-cloud Ignored Ignored Oct 24, 2025 6:28am
rivet-inspector Ignored Ignored Preview Oct 24, 2025 6:28am
rivet-site Ignored Ignored Preview Oct 24, 2025 6:28am

Copy link
Member Author

NathanFlurry commented Oct 24, 2025


How to use the Graphite Merge Queue

Add the label merge-queue to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

Base automatically changed from 10-23-fix_cloudflare-workers_fix_types to main October 24, 2025 06:28
@NathanFlurry NathanFlurry merged commit 260fe42 into main Oct 24, 2025
7 of 11 checks passed
@NathanFlurry NathanFlurry deleted the 10-23-fix_scripts_fix_fern_gen_path branch October 24, 2025 06:29
@claude
Copy link

claude bot commented Oct 24, 2025

PR Review: fix(scripts): fix fern gen path

Summary

This PR fixes the Fern SDK generator output path configuration, resulting in regenerated SDK files and error artifacts. The changes are primarily auto-generated files with minimal manual modifications.

Code Quality & Best Practices ✅

Positive aspects:

  1. Focused fix: The PR addresses a specific issue with the Fern generator path configuration
  2. Consistent regeneration: All affected files (SDKs, error JSONs) were properly regenerated
  3. Build tool configuration: Updated biome.json to exclude generated SDK files from linting/formatting

Observations:

  • The vast majority of changes (6,537 additions, 7,686 deletions across 100+ files) are auto-generated
  • The actual manual changes appear to be limited to:
    • engine/sdks/api/fern/generators.yml: 1 line changed (the path fix)
    • biome.json: ~3 lines changed (likely added exclusions)
    • Error JSON files: Reformatted (67 files with consistent 4-line changes)

Potential Issues & Concerns ⚠️

  1. Missing commit details: The PR body is empty. It would be helpful to document:

    • What was the incorrect path?
    • What is the corrected path?
    • Why did this cause issues?
    • What symptoms led to discovering this bug?
  2. Script inconsistency (scripts/fern/gen_inner.sh:10):
    The script references engine/sdks/typescript/api-FERN_GROUP but the generators.yml uses a relative path. While they should resolve to the same location, verify that the script works correctly with the new path.

  3. Error JSON formatting changes: All 67 error JSON files show 4 additions/4 deletions, suggesting they were completely regenerated or reformatted. Questions:

    • Is this reformatting intentional?
    • Should these files be in .gitignore if they are auto-generated?
    • The biome.json already excludes engine/artifacts, so why were they reformatted?

Performance Considerations ✅

  • No performance concerns identified
  • SDK generation is a build-time operation
  • The generated SDKs themselves show minimal changes (mostly 1-line updates)

Security Concerns ✅

  • No security issues identified
  • Changes are to configuration and generated code
  • No credential exposure or security-sensitive modifications

Test Coverage ⚠️

Questions:

  1. Were the SDKs tested after regeneration to ensure they work correctly?
  2. Are there integration tests that use these SDKs that passed?
  3. Did you verify the Fern generation script runs successfully with the new path?

Recommendation: Add a comment showing:

  • ./scripts/fern/gen.sh runs without errors
  • Example of using the generated SDK to verify it works

Architecture & Consistency 📋

From CLAUDE.md, the expected workflow is followed. The commit message uses the conventional commit format: fix(scripts): fix fern gen path ✅

Recommendations

  1. Add PR description: Document what path was wrong and what it is corrected to
  2. Verify script compatibility: Test that scripts/fern/gen_inner.sh works with the updated generators.yml path
  3. Add verification: Comment with evidence that SDK generation and tests pass
  4. Consider .gitignore: If error JSONs in engine/artifacts/errors/ are auto-generated, consider adding them to .gitignore
  5. Biome exclusion verification: The biome.json already excludes engine/artifacts, so the error JSON reformatting seems unexpected—verify the exclusion is working correctly

Verdict: ✅ Approve with minor suggestions

The core fix appears sound, but additional context and verification would increase confidence. The large diff is expected for a generator path fix that requires regeneration.


Review generated by Claude Code

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