Skip to content

test(review): cover Windows incident preservation - #2110

Open
dnlrsls wants to merge 2 commits into
Gentleman-Programming:mainfrom
dnlrsls:test/2076-incident-preservation
Open

test(review): cover Windows incident preservation#2110
dnlrsls wants to merge 2 commits into
Gentleman-Programming:mainfrom
dnlrsls:test/2076-incident-preservation

Conversation

@dnlrsls

@dnlrsls dnlrsls commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

🔗 Linked Issue

Closes #2076


🏷️ PR Type

  • type:bug — Bug fix
  • type:feature — New feature
  • type:docs — Documentation only
  • type:refactor — Code refactoring
  • type:chore — Build, CI, or tooling
  • type:breaking-change — Breaking change

📝 Summary

  • Add direct regression coverage for exact raw reviewer incident preservation.
  • Prove replay safety, distinct payload handling, and Windows directory-sync permission compatibility.
  • Retain fail-closed behavior for equivalent non-Windows sync failures.

📂 Changes

File / Area What Changed
internal/cli/review_incident_test.go Add direct exact-byte, replay, distinct-payload, and Windows sync compatibility tests.

🧪 Test Plan

Focused unit tests

go test ./internal/cli -run 'TestPreserveIncidentArtifact(PublishesExactPrivateBytes|KeepsWindowsDirectorySyncPermissionFailClosed)$' -count=1
go vet ./internal/cli
go run ./internal/gofmtcheck
  • Focused unit tests pass.
  • Go format check passes.
  • Full repository unit suite: CI will run it; the local full CLI package has unrelated Windows fixture failures and a Git subprocess timeout.
  • E2E: N/A; this is test-only coverage for a native filesystem helper.
  • Benchmark validation: N/A; this test-only slice changes no benchmarked product behavior.

🤖 Automated Checks

Check Status
Cognitive load 75 / 400 changed lines
Approved issue status:approved present
Type label type:chore
CI Pending

✅ Contributor Checklist

  • PR is linked to issue fix(opencode): provider-bound review inspection is denied on Windows #2076 with status:approved.
  • PR stays within 400 changed lines.
  • Exactly one type:* label will be applied.
  • Focused unit tests pass.
  • Go format passes.
  • Benchmark validation is not applicable, as explained above.
  • Documentation changes are not required for test-only coverage.
  • Commit follows Conventional Commits and has no Co-Authored-By trailer.

💬 Notes for Reviewers

The original live preservation failure did not reproduce on this Windows filesystem. This PR therefore adds direct proof for existing behavior without claiming a production fix.

Chain Context

Field Value
Chain Windows OpenCode reviewer inspection
Tracker PR Not needed
Position 2 of 2
Base main
Depends on #2109 for delivery order only; technically independent
Follow-up None
Review budget 75 / 400
Starts at Current origin/main
Ends with Direct Windows-relevant incident preservation regression coverage.

Chain Overview

main
 ├── #2109 — Windows inspection fix
 └── 📍 PR 2 — incident preservation coverage (this PR)

Scope

Autonomy

  • CI is expected to pass for this PR branch.
  • This PR has one deliverable scope.
  • This PR can be rolled back without unrelated changes.
  • Tests cover this unit.

Summary by CodeRabbit

  • Tests
    • Added coverage ensuring incident artifacts preserve their exact contents.
    • Added checks for deterministic replay and separation of distinct payloads.
    • Added platform-specific validation for directory synchronization permissions and fail-closed behavior.

@dnlrsls dnlrsls added the type:chore Maintenance, dependencies, tooling label Jul 31, 2026
@coderabbitai

coderabbitai Bot commented Jul 31, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 8389cb6c-4e6c-4079-aa22-83d492a800c8

📥 Commits

Reviewing files that changed from the base of the PR and between 8a6a950 and 6b1c3ad.

📒 Files selected for processing (1)
  • internal/cli/review_incident_test.go

📝 Walkthrough

Walkthrough

The test suite adds coverage for exact incident-artifact preservation, deterministic replay, distinct payload handling, and platform-specific directory-sync permission errors.

Changes

Incident artifact validation

Layer / File(s) Summary
Artifact replay and payload separation
internal/cli/review_incident_test.go
The tests verify exact private-byte preservation, deterministic replay for identical inputs, and separate artifacts for different payloads.
Platform-specific durability handling
internal/cli/review_incident_test.go
The tests verify that Windows directory-sync permission failures are tolerated and equivalent Linux failures remain errors.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related issues

  • Gentleman-Programming/gentle-ai#1983: Covers related Windows-specific permission and error handling in internal/cli tests.

Possibly related PRs

Suggested labels: slop

Suggested reviewers: alan-thegentleman, matam15

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Linked Issues check ⚠️ Warning The tests cover the reported paths, but the PR changes no production behavior and does not implement issue #2076's runtime fix. Implement the provider-bound Windows inspection and raw-result preservation fix, then retain these regression tests.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the added Windows incident-preservation regression tests.
Out of Scope Changes check ✅ Passed The changes are limited to regression tests for Windows incident preservation and related synchronization behavior.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot 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.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@internal/cli/review_incident_test.go`:
- Around line 57-59: Update the replay test around preserveIncidentArtifact to
capture its returned artifact, then assert that the replay result has the same
Path and SHA256 values as artifact; retain the existing failure handling for
replay errors.
- Around line 70-76: Update
TestPreserveIncidentArtifactKeepsWindowsDirectorySyncPermissionFailClosed so its
injected sync failure uses os.ErrPermission directly in the os.PathError instead
of syscall.Errno(5), ensuring OS-independent permission matching. Remove the
now-unused syscall import.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f1f769c4-8147-4682-b055-ea4767d5b578

📥 Commits

Reviewing files that changed from the base of the PR and between fdecaf5 and 8a6a950.

📒 Files selected for processing (1)
  • internal/cli/review_incident_test.go

Comment thread internal/cli/review_incident_test.go Outdated
Comment thread internal/cli/review_incident_test.go
@dnlrsls

dnlrsls commented Jul 31, 2026

Copy link
Copy Markdown
Contributor Author

Ready as the second PR in the chain. All CI, runtime, E2E, cognitive-load, approval, and CodeRabbit checks pass. Please merge this after #2109. @Alan-TheGentleman @MataM15, could one of you complete the second merge? Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type:chore Maintenance, dependencies, tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(opencode): provider-bound review inspection is denied on Windows

1 participant