Skip to content

Fix stale presenter after expected operation errors#2589

Merged
GCHQDeveloper581 merged 3 commits into
gchq:masterfrom
zainnadeem786:fix/render-pdf-error-presentation
Jun 24, 2026
Merged

Fix stale presenter after expected operation errors#2589
GCHQDeveloper581 merged 3 commits into
gchq:masterfrom
zainnadeem786:fix/render-pdf-error-presentation

Conversation

@zainnadeem786

@zainnadeem786 zainnadeem786 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix an issue where an expected OperationError or DishError could be rendered using a stale presenter from a previously successful operation.

This was reproducible with the following chain:

Render PDF
↓
Generate QR Code (OperationError)

where the expected plain error message was incorrectly returned as an HTML PDF iframe.

Root Cause

Recipe.execute() resets lastRunOp at the beginning of execution and updates it after each successful operation.

When an operation throws an expected OperationError or DishError, the error message is written to the dish as a "string" and execution returns early, but lastRunOp is left unchanged.

Chef.bake() subsequently calls Recipe.present(), which uses the stale presenter from the previous successful operation. As a result, the error string can be wrapped using an unrelated presenter instead of being returned as plain text.

Fix

Clear the presentation state on the expected error paths by resetting lastRunOp before returning.

This keeps the error dish unpresented and allows the plain error message to be returned as intended.

The fix is generic and is applied to both:

  • OperationError
  • DishError

without introducing any special handling for individual operations.

Regression Tests

Added focused regression coverage for the reported scenario:

  • Render PDF followed by Generate QR Code returns a plain QR error message.
  • The resulting output does not contain a PDF <iframe>.

Validation

Executed the relevant test suites:

Node API tests:
PASSING 250/250

Operation tests:
PASSING 2051/2051

Impact

@CLAassistant

CLAassistant commented Jun 22, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@GCHQDeveloper581 GCHQDeveloper581 enabled auto-merge (squash) June 24, 2026 16:27

@GCHQDeveloper581 GCHQDeveloper581 left a comment

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.

Looks good!

Thanks for your contribution.

Checked:

  • new test fails on previous code
  • web gui still correctly indicates which step in the recipe has (first) encountered an error
  • recipe still terminates at the filing step
  • web gui now renders (error message) correctly for this test case

@zainnadeem786

Copy link
Copy Markdown
Contributor Author

@GCHQDeveloper581 Thanks for the review and for improving the tests. I really appreciate the feedback and the collaboration. Looking forward to contributing more!

@GCHQDeveloper581 GCHQDeveloper581 merged commit 0e50d32 into gchq:master Jun 24, 2026
2 checks passed
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.

bug(Render PDF, Chained): chaining render pdf with generate qr code will result on html output

3 participants