fix(output): write reports atomically

[dvd233]

Description

lazyFileWriter currently truncates an existing --output file as soon as the first byte is written. A write failure or forced termination during rendering can therefore leave JSON/SARIF consumers with a partial document at a path that previously held a complete report.

This change restores the old-or-new file guarantee by:

• writing to a collision-resistant temporary file in the target directory;
• preserving os.Create-style permissions for new files and the mode of an existing target;
• preserving write-through behavior for symlinked output paths;
• syncing and closing the temporary file before atomically renaming it into place;
• removing temporary files after write, sync, close, or rename failures; and
• printing Results written to only after the rename succeeds.

The cancellation policy is unchanged: a successfully rendered cancelled-run report can still replace an older report. This PR only addresses atomic persistence.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

• make test passes locally
• Manual testing (described below)

Validated on Windows/amd64 with Go 1.25.5:

• make fmt
• make check
• make test (full race-enabled suite)
• make build
• make coverage (90.5% total; 90% threshold)
• real ocr review --preview --format json --output replacement, including JSON parsing and success-hint verification
• forced process termination after writing the temporary file but before Close; the previous target remained byte-for-byte intact and only the identifiable .ocr-out-* temporary file contained the uncommitted bytes

Tests cover commit-on-close, replacement timing, new/existing file modes, rename failure, injected write failure, symlink preservation, ANSI stripping, unchanged JSON bytes, and temporary-file cleanup.

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (not applicable: no CLI syntax or configuration changed)
• I have signed the CLA

Related Issues

Addresses item 1 of #1141.

[github-actions]

✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s).

[wu21-web]

@dvd233

[wu21-web]

Suggested change

	if err := os.Rename(w.tempPath, w.commitPath); err != nil {
	if err := replaceOutputFile(w.tempPath, w.commitPath); err != nil {

[dvd233]

Thanks. replaceOutputFile doesn't currently exist in this package or elsewhere in the repository. Is there a specific implementation or platform behavior you want it to provide beyond os.Rename? Go's Windows implementation calls MoveFileEx with MOVEFILE_REPLACE_EXISTING, and TestLazyFileWriter_ReplacesExistingFileOnlyOnClose passes in the current Windows CI job. I can add a wrapper if it serves a concrete semantic or testing purpose, but I don't want to introduce a pass-through helper based on the name alone.

[lizhengfeng101]

The temp+rename approach is right, but defer closeOut() (review_cmd.go:118) registers first and therefore runs last — so the rename now happens after the MCP shutdown loop, widening the SIGKILL window from milliseconds to the ~15s/server that issue #1141 item 2 says will overrun docker stop, which turns a rare truncated report into a routinely lost one (and invalidates that item's "the report [is] already on disk" premise); calling closeOut() explicitly right after emitRunResult succeeds and keeping the defer as a fallback fixes it for free, since Close is already idempotent.

[dvd233]

Thanks for catching the defer ordering. Fixed in 7ae6442:

• finishOutput now commits the report immediately after emitRunResult succeeds, before deferred MCP shutdown.
• The defer remains as an exact-once fallback for early returns and emit failures; close errors still propagate.
• Added TestReviewOutputCommittedBeforeMCPShutdown, which reads and JSON-decodes the output at the MCP shutdown boundary, so the old defer order fails deterministically.

Validation:

• make check
• make test (-race)
• make coverage — 90.4% (90% threshold)
• make build
• all current GitHub CI and CodeQL checks pass