Skip to content

fix: surface output.postProcess execution errors#3683

Open
Copilot wants to merge 5 commits intomainfrom
copilot/fix-output-postprocess-error
Open

fix: surface output.postProcess execution errors#3683
Copilot wants to merge 5 commits intomainfrom
copilot/fix-output-postprocess-error

Conversation

Copy link
Copy Markdown
Contributor

Copilot AI commented Apr 3, 2026

  • Fix postprocessOutput in packages/shared/src/config/output/postprocess.ts to check result from sync() and throw meaningful errors when:
    • result.error is set (e.g., ENOENT when command is not found in PATH)
    • result.status is non-zero (command exited with failure), including stderr in the message
  • Refactor non-zero status error message to use let to avoid duplicating the base message string
  • Add unit tests in packages/shared/src/config/output/__tests__/postprocess.test.ts covering the new error-handling behavior (10 new tests, all passing)
  • Run lint and tests to verify changes (all tests pass, lint clean)

Copilot AI linked an issue Apr 3, 2026 that may be closed by this pull request
@bolt-new-by-stackblitz
Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@pullfrog
Copy link
Copy Markdown

pullfrog bot commented Apr 3, 2026

Reviewed PR #3683 — it's a WIP draft with zero code changes (the only commit is an empty "Initial plan" commit). No code to review yet; will re-review once implementation commits are pushed.

Pullfrog  | View workflow run | Triggered by Pullfrog𝕏

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 3, 2026

🦋 Changeset detected

Latest commit: df406de

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

This PR includes changesets to release 3 packages
Name Type
@hey-api/openapi-ts Patch
@hey-api/shared Patch
@hey-api/openapi-python 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
Copy Markdown

vercel bot commented Apr 3, 2026

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

Project Deployment Actions Updated (UTC)
hey-api-docs Ready Ready Preview, Comment Apr 3, 2026 6:35pm

Request Review

Copilot AI changed the title [WIP] Fix output.postProcess execution error being swallowed fix: surface output.postProcess execution errors Apr 3, 2026
Copilot AI requested a review from mrlubos April 3, 2026 13:27
@mrlubos
Copy link
Copy Markdown
Member

mrlubos commented Apr 3, 2026

@copilot avoid duplicating hard-coded message string for non-zero status codes, use let instead of const to avoid duplication

Copy link
Copy Markdown
Contributor Author

Copilot AI commented Apr 3, 2026

@copilot avoid duplicating hard-coded message string for non-zero status codes, use let instead of const to avoid duplication

Done in 329d983 — refactored to use let so the base message is built once and stderr is conditionally appended.

Fixes issues related to surfacing postprocess errors in the output.
@mrlubos mrlubos marked this pull request as ready for review April 3, 2026 15:51
@pullfrog
Copy link
Copy Markdown

pullfrog bot commented Apr 3, 2026

Reviewed PR #3683. The core error detection logic is correct and tests are thorough. Flagged two issues: (1) throwing plain Error instead of ConfigError causes crash report + GitHub issue prompt for what is a user configuration problem, and (2) a minor edge case where signal-killed processes produce a confusing "exited with code null" message.

Task list (4/4 completed)
  • Read the PR diff to understand changes
  • Read the full source files for context
  • Review linked issue for context
  • Draft and submit review

Pullfrog  | View workflow run | Triggered by Pullfrog𝕏

@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. bug 🔥 Broken or incorrect behavior. labels Apr 3, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Apr 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.47%. Comparing base (549a42e) to head (df406de).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3683      +/-   ##
==========================================
+ Coverage   39.39%   39.47%   +0.07%     
==========================================
  Files         520      521       +1     
  Lines       19279    19299      +20     
  Branches     5708     5722      +14     
==========================================
+ Hits         7595     7618      +23     
+ Misses       9445     9442       -3     
  Partials     2239     2239              
Flag Coverage Δ
unittests 39.47% <100.00%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@pullfrog
Copy link
Copy Markdown

pullfrog bot commented Apr 3, 2026

TL;DRpostprocessOutput silently discarded spawn failures and non-zero exit codes from user-configured post-processors (e.g. prettier, biome). This PR captures the cross-spawn result and throws descriptive errors so users actually see what went wrong.

Key changes

  • Add error handling to postprocessOutput — checks result.error (spawn failure) and result.status (non-zero exit), throwing with the processor name, exit code, and stderr when available.
  • Add unit tests for postprocessOutput — 10 new tests covering happy paths, ENOENT spawn errors, non-zero exits with/without stderr, preset resolution, and fail-fast ordering.
  • Add changeset — patch bump for @hey-api/openapi-ts and @hey-api/shared.

Summary | 3 files | 4 commits | base: maincopilot/fix-output-postprocess-error


Error handling for post-processor execution

Before: sync() return value was ignored — a missing binary or a formatter crash produced no error, and generation appeared to succeed.
After: spawn failures throw Post-processor "<name>" failed to run: <message> and non-zero exits throw Post-processor "<name>" exited with code <N> with stderr appended when present.

The fix captures the cross-spawn sync() result and checks two conditions: result.error (set when the binary cannot be spawned at all, e.g. ENOENT) and result.status !== 0 (command ran but returned a failure code). A let-bound message string avoids duplicating the base text when appending stderr.

packages/shared/src/config/output/postprocess.ts · packages/shared/src/config/output/__tests__/postprocess.test.ts

Pullfrog  | View workflow run | Triggered by Pullfrog𝕏

Copy link
Copy Markdown

@pullfrog pullfrog bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error detection logic is correct and the tests are thorough. Two issues worth addressing — one impacts user experience significantly, the other is a minor edge case.

Pullfrog  | Fix all ➔Fix 👍s ➔View workflow run𝕏

@mrlubos
Copy link
Copy Markdown
Member

mrlubos commented Apr 3, 2026

@pullfrog implement your feedback. Also have a look at the failing CI, seems there was another bug lurking in the postprocessor implementation. Can you figure out how to fix that too?

@pullfrog
Copy link
Copy Markdown

pullfrog bot commented Apr 3, 2026

Addressed all three review comments and fixed the CI failure. The review feedback (use ConfigError instead of Error, guard result.status !== null for signal-killed processes) is implemented. The CI bug was caused by post-processors (Oxfmt, ESLint) running against empty output directories — fixed by adding an early return in postprocessOutput when the directory doesn't exist or is empty. All 457 tests pass, lint and typecheck clean.

Task list (6/6 completed)
  • Address review: use ConfigError instead of plain Error
  • Address review: guard against null status (signal-killed processes)
  • Fix CI: post-processor errors on empty output directories
  • Update tests to match changes
  • Build, lint, and test
  • Commit, push, and reply to review comments

Pullfrog  | View workflow run | Triggered by Pullfrog𝕏

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

Labels

bug 🔥 Broken or incorrect behavior. size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

output.postProcess execution error is swallowed

2 participants