Skip to content

fix(cli): gracefully handle docs resolution failures in missing-redirects rule#15905

Open
paarthfern wants to merge 2 commits into
mainfrom
devin/1778783591-fix-missing-redirects-crash
Open

fix(cli): gracefully handle docs resolution failures in missing-redirects rule#15905
paarthfern wants to merge 2 commits into
mainfrom
devin/1778783591-fix-missing-redirects-crash

Conversation

@paarthfern
Copy link
Copy Markdown
Contributor

@paarthfern paarthfern commented May 14, 2026

Description

Follow-up to #15853 which added the core try-catch fix for the missing-redirects rule. This PR adds the assertNever exhaustiveness guard to the makeSkipVisitor switch statement per CLAUDE.md conventions, and includes a changelog entry.

Changes Made

  • Added default: assertNever(fetchResult) to the makeSkipVisitor switch on the FetchResult discriminated union, ensuring compile-time safety when new variants are added
  • Added assertNever import from @fern-api/core-utils
  • Added unreleased changelog entry for the fix

Testing

  • All 145 unit tests in @fern-api/docs-validator pass
  • pnpm lint:biome passes
  • pnpm format passes
  • TypeScript compilation succeeds

Link to Devin session: https://app.devin.ai/sessions/9e60e54a91c84401a1d139ae7e2fb655
Requested by: @paarthfern


Open in Devin Review

@devin-ai-integration
Copy link
Copy Markdown
Contributor

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment and CI monitoring

Copy link
Copy Markdown

@claude claude Bot left a comment

Choose a reason for hiding this comment

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

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

Tip: disable this comment in your organization's Code Review settings.

Copy link
Copy Markdown
Contributor

@devin-ai-integration devin-ai-integration Bot left a comment

Choose a reason for hiding this comment

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

Devin Review found 1 potential issue.

View 2 additional findings in Devin Review.

Open in Devin Review

Comment on lines +190 to +201
case "resolve-failed":
return {
file: () => [
{
severity: "warning",
message:
`Missing redirects check skipped: failed to resolve docs definition (${fetchResult.reason}). ` +
"This can happen after a major CLI upgrade that changes type naming. " +
"Publish your docs once to update the baseline, then re-run this check."
}
]
};
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Switch on discriminated union FetchResult lacks default: assertNever() for exhaustiveness

The makeSkipVisitor function switches on fetchResult.type (a discriminated union) but does not include a default case calling assertNever. Per CLAUDE.md: "Every switch statement on a discriminated union's type field must include a default case [that] call[s] assertNever from @fern-api/core-utils." The same package already follows this pattern in validateDocsWorkspace.ts:31-32. While TypeScript's return-type checking provides partial safety here, a new variant added to FetchResult without updating makeSkipVisitor would not produce a compile error in the default branch — only a potentially confusing implicit-undefined-return error.

Suggested change
case "resolve-failed":
return {
file: () => [
{
severity: "warning",
message:
`Missing redirects check skipped: failed to resolve docs definition (${fetchResult.reason}). ` +
"This can happen after a major CLI upgrade that changes type naming. " +
"Publish your docs once to update the baseline, then re-run this check."
}
]
};
case "resolve-failed":
return {
file: () => [
{
severity: "warning",
message:
`Missing redirects check skipped: failed to resolve docs definition (${fetchResult.reason}). ` +
"This can happen after a major CLI upgrade that changes type naming. " +
"Publish your docs once to update the baseline, then re-run this check."
}
]
};
default:
assertNever(fetchResult);
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Good catch — added default: assertNever(fetchResult) and the assertNever import in 789f56e.

devin-ai-integration Bot and others added 2 commits May 14, 2026 22:34
…ects rule

The missing-redirects rule called DocsDefinitionResolver.resolve() without
error handling. When resolution failed (e.g. after a major CLI upgrade that
changes inline type naming), the thrown TaskAbortSignal propagated as a
fatal violation with an uninformative error message.

Wrap the resolution and navigation-building code in a try-catch so the rule
gracefully skips with a descriptive warning instead of crashing.

Co-Authored-By: paarth <paarth@buildwithfern.com>
Co-Authored-By: paarth <paarth@buildwithfern.com>
@devin-ai-integration devin-ai-integration Bot force-pushed the devin/1778783591-fix-missing-redirects-crash branch from 789f56e to 00ae45f Compare May 14, 2026 22:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant