Skip to content

fix(redirect): guard terminal push so a fault can't cascade through print#28

Merged
dsent merged 2 commits into
aldum:devfrom
Vadim1987:Vadim1987/fix-print-redirect-crash
Jul 2, 2026
Merged

fix(redirect): guard terminal push so a fault can't cascade through print#28
dsent merged 2 commits into
aldum:devfrom
Vadim1987:Vadim1987/fix-print-redirect-crash

Conversation

@Vadim1987

Copy link
Copy Markdown

magicPrint's M.output:push was unprotected; since IDE error paths call print, one terminal push fault would throw and cascade through every later print. Wrap push in pcall — origPrint already echoed the output above, so nothing is lost.

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the print redirection layer so failures in the model output channel don’t crash (and repeatedly re-crash) subsequent printing, which is especially important when IDE/error-handling paths rely on print.

Changes:

  • Wrap M.output:push(out) in pcall to prevent exceptions from propagating through _G.print.
  • Preserve existing behavior of still emitting output via origPrint(out) even when the model push fails.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@dsent dsent requested a review from aldum June 25, 2026 20:32
Comment thread src/model/io/redirect.lua Outdated
end
origPrint(out)
M.output:push(out)
pcall(M.output.push, M.output, out)

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

Should we do something about the possible error returned by pcall?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Vadim1987 any suggestions?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think it's fine to ignore it here. With on my opinion, reason:reason: origPrint already emitted the line right above, so a failed push doesn't lose any output. and the only channel we have to report the error is print itself — which is magicPrint → the same push → most likely the same failure. so acting on the error would probably just re-trigger the cascade the pcall is there to stop. That's why I left it swallowed. I can name the ignored value _ to signal it's intentional, not an oversight. if you'd rather log it somewhere out-of-band (a channel that doesn't go through print), that'd be the one way to surface it without the loop, if you think it's worth it.

@dsent dsent Jul 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@Vadim1987 let's assign to _ with a comment like "origPrint above has already emitted an error, if there was any" or similar so the intent is clear

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done — assigned to _ with a comment. The intent is exactly that: origPrint has already emitted the output (and the error text, if the failing thing was an error path), so the push failure has nothing left to report and print must never throw.

@dsent dsent merged commit 1655358 into aldum:dev Jul 2, 2026
8 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.

4 participants