Skip to content

fix: surface TOC extraction failures instead of writing a partial toc… - #303

Open
MuhammadRafay1 wants to merge 3 commits into
devfrom
toc-command-bug-fix
Open

fix: surface TOC extraction failures instead of writing a partial toc…#303
MuhammadRafay1 wants to merge 3 commits into
devfrom
toc-command-bug-fix

Conversation

@MuhammadRafay1

Copy link
Copy Markdown

The issue

portal toc new treated every failure to extract endpoint data as a reason to fall back to the default TOC:

if (result.isErr()) {
  this.prompts.fallingBackToDefault();
  return TocComponents.empty();
}

TocComponents.empty() means "no Endpoints, Events or Models sections". So when a user was logged out — or hit a network error, an expired key, or an invalid spec — the command would:

  1. Write a toc.yml containing only custom content, overwriting a possibly valid existing TOC,
  2. Report success, giving no indication that the endpoint sections were dropped, and
  3. Hang instead of returning to the shell.

The hang had a separate cause. generateTocData is a callAsStream endpoint, so on failure the SDK hands back an undrained IncomingMessage on ApiError.body. Its socket stays open and keeps the Node event loop alive, and since outro() only sets process.exitCode — we never call process.exit() — the process printed its outro and then sat there.

The fix

1. Propagate the error instead of silently degrading

actions/portal/toc/new-toc.ts

The inner IIFE now returns Result<TocComponents, ServiceError>. A failed extraction returns err(...); the caller reports the reason (auth, network, invalid spec) and returns ActionResult.failed(), leaving toc.yml untouched.

The genuinely-expected case is preserved: no spec/ directory still falls back to the default TOC, because there is nothing to extract and the default is the correct output. Only a failed extraction is now fatal. specFileStream.close() moved into a finally so it is released on both paths.

2. Release stream error bodies

infrastructure/service-error.ts

New exported discardStreamBody(error), called from handleServiceError after mapApiError has run, so mapping still reads error.result. Non-stream bodies are strings or Blobs with no destroy, so the check is typeof body?.destroy === "function".

Applied in the two services that catch ApiError without routing through handleServiceError:

  • transformation-service.ts — in a finally, so the 401 branch can still JSON.parse the body before it is discarded.
  • validation-service.ts — before the switch, since no branch there reads the body.

PortalService.generatePortal's 422 path is deliberately untouched: it returns error.body as the validation report at portal-service.ts:110-111, short-circuiting before handleServiceError, so that stream is never destroyed.

3. Renamed prompt method

PortalNewTocPrompts.logErrortocExtractionFailed, to name what it reports.

Behavior change

portal toc new now fails rather than writing a partial TOC when endpoint extraction fails — including when the user is logged out. Previously it exited 0 with a degraded file. PortalNewTocAction is only used by commands/portal/toc/new.ts, so nothing else is affected.

….yml

`portal toc new` fell back to the default TOC whenever endpoint extraction
failed, so a logged-out user (or any auth/network/spec error) got a toc.yml
missing its Endpoints, Events and Models sections written over a possibly
valid file, reported as success. Propagate the ServiceError instead: report
the reason and leave the file alone. The no-spec-directory case is still an
expected fallback, not a failure.

The command also hung after printing its outro: stream endpoints hand back an
undrained IncomingMessage on ApiError.body, whose open socket keeps the event
loop alive since outro() only sets process.exitCode. Discard it centrally in
handleServiceError, after the error has been mapped, plus in the two services
that catch ApiError without going through it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
if (apiError.statusCode === 400) {
return "Your API Definition is invalid. Please use the APIMatic VS Code Extension to fix the errors and try again.";
} else if (apiError.statusCode === 401) {
const message = JSON.parse(apiError.body as string).message;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Unguarded JSON.parse on an Error Path

The following line performs an unguarded JSON parse on an error path:

const message = JSON.parse(apiError.body as string).message;

There are three common scenarios where this throws:

Input Result
Stream body (401 from downloadTransformedFile, a callAsStream endpoint) JSON.parse stringifies the IncomingMessage to "[object Object]"SyntaxError (confirmed)
Empty body JSON.parse("")SyntaxError
Non-JSON body (gateway/proxy HTML, e.g. a corporate MITM 401 page) SyntaxError

The third case is the realistic one. Enterprise users behind an authenticating proxy often receive an HTML error page instead of JSON.

Consequence

The SyntaxError escapes handleTransformationErrors, then escapes the catch block in transformViaFile (because the throw occurs inside err(await this.handleTransformationErrors(error))), and ultimately surfaces as an unhandled rejection.

As a result:

  • Users receive a raw stack trace instead of the intended error message.
  • outro(result) is skipped.
  • The process exits with the wrong exit code.
  • Failure telemetry is not recorded correctly.

Severity

Medium — this does not cause data loss or corruption, but it degrades an already-failing path into an unhandled exception and breaks expected error reporting.

Why This Belongs in This PR

Although this issue is pre-existing, this PR modifies these exact lines by wrapping them in try/finally and adds a comment asserting that this branch safely reads the body.

Additionally:

  • The safe-parse pattern already exists in the sibling file (validation-service.ts, lines 225–234).
  • unauthorizedWithHint already accepts string | null.

This is a small, low-risk fix (roughly five lines) that aligns this code path with the existing implementation.

MuhammadRafay1 and others added 2 commits August 4, 2026 14:27
`JSON.parse(apiError.body as string)` assumed a JSON string body. A stream
endpoint hands back an `IncomingMessage` (stringified to "[object Object]"),
an empty body parses to nothing, and an authenticating proxy can answer with
an HTML error page — each throws a SyntaxError. It escaped
`handleTransformationErrors` and the catch in `transformViaFile`, because the
call sits inside the `err(await ...)` that catch builds, so it surfaced as an
unhandled rejection: a raw stack trace instead of the message, no outro, wrong
exit code, and no failure telemetry.

Parse defensively and fall back to `unauthorizedWithHint`'s own default
message, matching the safe-parse pattern already in `validation-service`.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The generator only enumerates endpoints, models and events when it has data:
`getEndpointsSection` short-circuits on `data.size === 0` before it looks at
the expand flag, and the models and events sections drop out entirely. So
`portal toc new --expand-endpoints` against a build directory with no `spec/`
wrote a collapsed TOC, ignored the flag and exited 0 — the flag help text
documents the requirement but nothing enforced it.

Fail instead when any expand flag is set and `spec/` is absent, naming the
flags that cannot be honoured. Checked before the overwrite prompt so a run
that cannot succeed does not ask to replace a file first.

Unchanged: with no expand flags a missing `spec/` still falls back to the
default TOC, since that is the correct output when there is nothing to expand.
Auth is deliberately not part of this check — with no spec no request is made,
so reporting an unauthorized error here would name a cause that did not occur.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@sonarqubecloud

sonarqubecloud Bot commented Aug 4, 2026

Copy link
Copy Markdown

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