fix(export): surface checkValidRev error message on bad :rev

[JohnMcLear]

Summary

/p/:pad/:rev/export/:type with a non-numeric :rev (e.g. test1) was returning a 500 with Express's default HTML error page (<title>Error</title> … <pre>Internal Server Error</pre>). checkValidRev does throw a CustomError('rev is not a number', 'apierror'), but the route handler's .catch(next) handed the error to Express's default renderer and the message never reached the body. Callers had no way to tell why the export failed.

Catch the apierror in the route handler and send the message as a text/plain 500 body. All other error names still go through next(err) so we don't change behaviour for unrelated failures.

Also retitle the two affected tests — they said "is 403" while asserting expect(500), a leftover from an earlier expectation.

Test plan

tests/backend/specs/api/importexportGetPost.ts:

• Before: 2 failing (txt request rev test1, html request rev test1).
• After: 58 passing, 0 failing.

The fix is narrow: it only intercepts errors whose name === 'apierror'. Anything else continues to propagate to Express's error handler.

Note: these failures are invisible in CI today because of the broken backend test glob — see #7789. That has to land for the green checks on this PR to actually run the affected tests.

Fixes #7788

🤖 Generated with Claude Code

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

Review Summary by Qodo

Surface checkValidRev error message in export response body

🐞 Bug fix

Walkthroughs

Description

• Surface checkValidRev error message in export response body
• Non-numeric :rev parameter now returns plain-text 500 with error message
• Prevents Express default HTML error page from being returned
• Retitle test to accurately reflect expected 500 status code

Diagram

flowchart LR
  A["Non-numeric :rev parameter"] -->|"checkValidRev throws apierror"| B["Route handler catches error"]
  B -->|"err.name === apierror"| C["Send text/plain 500 with message"]
  B -->|"other errors"| D["Propagate to Express handler"]

Loading

File Changes

1. src/node/hooks/express/importexport.ts 🐞 Bug fix +9/-1

Catch apierror and return plain-text response

• Modified error handler to catch apierror exceptions from checkValidRev
• Returns plain-text 500 response with error message instead of propagating to Express
• Checks err.name === 'apierror' and !res.headersSent before sending response
• Other errors continue to propagate through next(err) for existing behavior

src/node/hooks/express/importexport.ts

---

2. src/tests/backend/specs/api/importexportGetPost.ts 🧪 Tests +1/-1

Correct test title to match assertion

• Retitled test from "txt request rev test1 is 403" to "txt request rev test1 returns 500 with error
 message"
• Test title now accurately reflects the expected 500 status code assertion
• Removes misleading 403 reference from test description

src/tests/backend/specs/api/importexportGetPost.ts

---

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0)

1. Non-apierror export errors unhandled ✓ Resolved 📎 Requirement gap ☼ Reliability

Description

The export route only converts apierror exceptions into a deterministic plain-text response; all
other export errors are passed to next() and can still fall back to Express's default HTML error
page. This can reintroduce non-deterministic HTML error bodies for other export failures.

Code

src/node/hooks/express/importexport.ts[R74-82]

+    })().catch((err) => {
+      // checkValidRev throws CustomError('...', 'apierror') for non-numeric or
+      // out-of-range :rev. Surface the message as a plain-text body so callers
+      // see why the request failed instead of Express's default HTML page.
+      if (err && err.name === 'apierror' && !res.headersSent) {
+        return res.status(500).type('text/plain').send(err.message);
+      }
+      return next(err || new Error(err));
+    });

Evidence

PR Compliance ID 2 requires export-route errors to be caught and returned with a deterministic,
non-HTML body that includes err.message. The added handler only returns a plain-text body for
apierror; for all other errors it calls next(err), allowing fallback to Express's default HTML
error response.

Export route has explicit error handling to prevent fallback to Express default HTML error page
src/node/hooks/express/importexport.ts[74-82]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The export route only special-cases `err.name === 'apierror'` and calls `next(err)` for everything else, which can still trigger Express's default HTML error renderer.
## Issue Context
Compliance requires explicit error handling for export-route failures so clients/tests get deterministic, non-HTML error bodies (at minimum containing `err.message`) rather than Express's default HTML page.
## Fix Focus Areas
- src/node/hooks/express/importexport.ts[74-82]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Error downloads as attachment ✓ Resolved 🐞 Bug ≡ Correctness

Description

If checkValidRev() throws, ExportHandler.doExport() has already set `Content-Disposition:
attachment`, so the new plain-text 500 body can still be delivered as a downloadable file rather
than a visible error. This makes the surfaced error message harder to notice and can confuse callers
about whether an export succeeded.

Code

src/node/hooks/express/importexport.ts[R78-80]

+      if (err && err.name === 'apierror' && !res.headersSent) {
+        return res.status(500).type('text/plain').send(err.message);
+      }

Evidence

doExport() sets the attachment download header before calling checkValidRev(), so invalid :rev
errors happen after Content-Disposition is already set; the new catch returns a plain-text error
without clearing that header.

src/node/handler/ExportHandler.ts[58-65]
src/node/utils/checkValidRev.ts[7-20]
src/node/hooks/express/importexport.ts[29-83]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`ExportHandler.doExport()` calls `res.attachment(...)` before validating `req.params.rev`. If `checkValidRev()` throws, the new `apierror` catch returns a plain-text 500 but keeps `Content-Disposition: attachment`, so clients (especially browsers) may download the error body as an export file.
## Issue Context
- `res.attachment()` is invoked before `checkValidRev()`.
- The route-level catch sends the error message but does not clear `Content-Disposition`.
## Fix Focus Areas
- src/node/hooks/express/importexport.ts[74-82]
- src/node/handler/ExportHandler.ts[58-65]
## Implementation notes
Prefer one of:
1) Move rev validation (`checkValidRev`) **before** `res.attachment(...)` in `ExportHandler.doExport()` so invalid `:rev` never sets download headers.
2) Or, in the `apierror` catch branch, call `res.removeHeader('Content-Disposition')` (and optionally clear any previously-set export `Content-Type`) before sending the plain-text 500.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---