Skip to content

fix(view): preserve native render failure diagnostics - #329

Open
y4ho0 wants to merge 2 commits into
iOfficeAI:mainfrom
y4ho0:agent/fix-native-render-error-diagnostics
Open

y4ho0 wants to merge 2 commits into
iOfficeAI:mainfrom
y4ho0:agent/fix-native-render-error-diagnostics

Conversation

@y4ho0

@y4ho0 y4ho0 commented Aug 20, 2026

Copy link
Copy Markdown
Contributor

Summary

  • preserve native PowerPoint and Word rendering exceptions instead of replacing every failure with an “Office is not installed” message
  • distinguish COM class-registration/application availability failures from document-open or rendering failures
  • retain the original failure stage and HRESULT in explicit --render native diagnostics
  • keep --render auto behavior unchanged: native failures still fall back silently to HTML

Why

When Office is installed and COM activation succeeds, a document-specific open failure such as Invoke(Open) hr=0x80070570 was swallowed by the native backend. The caller then reported that PowerPoint or Word was not installed, which sent users toward the wrong fix and hid the useful HRESULT.

The native backends now surface worker-thread failures while preserving their original stack. Explicit native mode maps those failures to either native_unavailable or native_render_failed; auto mode continues to catch them and use the existing HTML fallback.

Validation

  • dotnet build src/officecli/officecli.csproj -c Release — succeeded with 0 errors
  • targeted diagnostic harness covered:
    • non-Windows native request → native_unavailable
    • COM class-not-registered → native_unavailable with HRESULT
    • Invoke(Open) hr=0x80070570native_render_failed with the original stage and HRESULT
    • native backend returning no image → native_render_failed
  • verified that every production caller catches the surfaced backend exception, so --render auto retains its fallback behavior

Closes #326

@y4ho0
y4ho0 marked this pull request as ready for review August 20, 2026 14:14
@argszero

Copy link
Copy Markdown
Contributor

Independent verification of the claim this PR rests on — plus one boundary and a supersede notice

I have an independent patch for #326 posted in that thread (2026-09-20), so I read this PR against the same call graph. Three things worth putting on the record.

1 · "Every production caller catches the surfaced backend exception" — verified

This is the load-bearing claim: the backends used to answer every failure with a bare null, and --render auto is the default path, so a single unguarded caller would turn the HTML fallback into a failure for everyone.

Read at origin/main @ dced0d74, enumerating every call site of the four entry points this PR changes (.Render / .RenderGrid on both backends):

call site what it calls guarded by
CommandBuilder.View.cs:272 pptx grid try :265catch { directPng = null; } :279
CommandBuilder.View.cs:276 pptx single same try/catch
CommandBuilder.View.cs:357 docx grid inline try / catch :358
CommandBuilder.View.cs:384 docx single inline try / catch :385
ResidentServer.cs:1625, :1626 pptx grid / single (one ternary) try :1622catch :1628
ResidentServer.cs:1686 docx grid inline try / catch
ResidentServer.cs:1719 docx single inline try / catch

7/7 sit inside a catch, so the fallback holds on both the CLI and the resident path. Two facts make that exhaustive rather than merely plausible:

  • both classes are internal static (PowerPointPngBackend.cs:22, WordPdfBackend.cs:15), so no caller can exist outside the assembly;
  • git grep -l "PowerPointPngBackend\|WordPdfBackend" origin/main returns only CommandBuilder.View.cs, ResidentServer.cs, CommandBuilder.Refresh.cs (which uses RefreshFields — untouched here; I checked that no added or removed line in the diff mentions RefreshFields or GetPageCount) and the two backends themselves.

The repo also carries no test files (git ls-tree -r --name-only origin/main | grep -ci test → 0), so there is no test-side caller to update either.

I checked the no-regression half too, on the branch you did not mean to change: with nativeAttempted == false, Create(...) reproduces the previous sentence character-for-character (--render native requires Windows with Microsoft PowerPoint installed.), which is exactly what a macOS/Linux user sees from the resident. Good.

2 · On the resident path the code you build is discarded

Create returns a CliException carrying Code and Suggestion, and the three CLI sites throw it — so the envelope gets both. The three resident sites are Console.Error.WriteLine(Create(...).Message): native_unavailable / native_render_failed are computed and then dropped, so the resident client's JSON envelope still has no structured field to branch on. Not a regression — that path only ever printed text — but it reads as an accident rather than a decision, and it is the same boundary described in #350 and #411 for other diagnostics (MakeResponse already folds resident stderr into the envelope, so a code could ride along). Your call; flagging it so it is deliberate if you leave it.

3 · Supersede notice, to save you reading two patches

My inline patch in #326 covers the same ground (six sites; an out NativeRenderFailure? overload where you throw). Yours is the better shape — ExceptionDispatchInfo keeps the original stack, and the callers were already structured to catch. Treat #326's patch as superseded by this PR.

One process note, not a complaint: gh pr checks 329 reports no checks reported on the branch and the PR has no review — it has been open since 2026-08-20. Your local dotnet build result is the only signal on the record, so this comment is meant as the independent half of what a CI run would have given a reviewer.

Not a formal review — I have no review permissions on this repository — and there is no .NET SDK on this machine either, so the above is source-level verification of the call graph, not a build.

@y4ho0

y4ho0 commented Sep 21, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the call-site review. I agree the resident path should preserve the structured diagnostic rather than discard it.

The three explicit-native failure sites now throw the existing CliException into ProcessRequest's error handler. Resident JSON responses retain error.code and error.suggestion; text-mode failures also return a nonzero exit code instead of printing an error and exiting successfully. The auto/HTML paths are unchanged.

Validation on macOS / .NET 10: the regression harness loads the actual built assembly and invokes the CLI. It covers docx/pptx, single/grid, direct/resident, JSON/text, and successful auto/HTML screenshot controls. The old PR head fails 12 of 45 checks; the updated build passes all 45. Seven synthetic diagnostic cases also exercise the real classifier and JSON formatter, including COM activation/open errors, timeout and empty output. Release build succeeds with no new warnings. Real Windows Office/COM execution remains untested on this macOS host.

A minimal resident repro on macOS/Linux is officecli open sample.docx, then officecli view sample.docx screenshot --render native --json (now error.code: native_unavailable plus error.suggestion). Without --json, the same failure now exits nonzero. Close the session afterward with officecli close sample.docx.

@argszero

Copy link
Copy Markdown
Contributor

Re-verified on the updated head (53ca7ba7) — the diagnostic now survives the round trip

Source-level again, no SDK; same question as before (if this were false, which path breaks?).

  • The code and suggestion actually reach the envelope. NativeRenderDiagnostics.Create returns a CliException with Code (native_unavailable / native_render_failed) and Suggestion set (Core/NativeRenderDiagnostics.cs:17-55), and the resident's error handler builds the envelope through OutputFormatter.BuildErrorResult (Core/OutputFormatter.cs:282-289), which copies cli.Code / cli.Suggestion / cli.Help / cli.ValidValues for exactly that exception type. So view … --render native --json now carries error.code instead of a bare message — and it is the same formatter the non-resident CLI uses, not a parallel one that could drift.
  • Dropping the returns introduced no fall-through. Each throw sits inside the same if (renderMode == "native" && directPng == null) block that used to return, so the if (directPng == null) branch immediately below remains unreachable in that case. throw leaves the method exactly where return did; the only change is that the caller now learns about it.
  • It is caught, and in the right place. ExecuteView is called from ExecuteCommand (:1084), inside the try at :785, whose catch (Exception ex) is :902. Text mode takes MakeResponse(1, "", "Error: …") (:923); JSON takes MakeResponse(1, WrapErrorEnvelope(rendered), "") (:916) — the empty stderr argument means the message is not also duplicated into stderr, so a client sees one error, not two.
  • The nonzero status reaches the CLI process, not just the resident reply. TryResident returns the resident's exit code unchanged and view returns it directly (CommandBuilder.View.cs:136: }, json) is {} rc) return rc;). That is what makes the text-mode half of your claim hold at the boundary a script actually observes.
  • auto / HTML are untouched: every changed site is gated on renderMode == "native", and the diff is exactly those 3 hunks.

Nothing further from me on this one. Worth noting for the record: resident mode and standalone mode now agree on both the error code and the exit status for the same failure, which is the property the earlier call-site comment was asking for — and the same shape as the other resident-vs-standalone envelope gaps that have been raised in #350 and #411. If those are still open, this is the pattern to reuse rather than a new mechanism.

Not a formal review (no review permissions here), and no build — the above is the call graph at 53ca7ba7, not a run of your 45-check harness.

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.

v1.0.144: native render reports Office missing when COM document open fails

2 participants