fix(sandbox): self-heal a corrupt unelevated setup marker

[euxaristia]

Follow-up addressing the review feedback from coderabbitai and @anandh8x on #427 (merged): a malformed or truncated windows-unelevated-setup.json returned a hard error from loadWindowsUnelevatedSetupMarker, bricking every unelevated command until the file was hand-deleted, while the unknown-schema path already self-healed by resetting to an empty marker.

What

Treat corrupt JSON exactly like an unknown schema: reset to an empty current-schema marker so the runner re-applies the ACL plan and rewrites the file on the next command.

Why it's safe

The marker only memoizes idempotent ACL applies. Resetting it can only cost redundant work (one extra apply); it can never skip enforcement, so failing open on the memoization is the fail-safe direction.

Testing

• New TestLoadWindowsUnelevatedSetupMarkerSelfHealsCorruptFile: a truncated marker body loads as an empty marker without error, and a subsequent record over the corrupt file succeeds.
• go test ./internal/sandbox/ green on Windows 11, go vet clean.

🤖 Generated with Claude Code

[coderabbitai]

Walkthrough

loadWindowsUnelevatedSetupMarker now treats a JSON unmarshal failure on the marker file as an empty marker state (current schema version, nil error) instead of returning a parse error, causing callers to re-apply and rewrite the marker. A test covers this self-healing behavior.

Changes

Marker Self-Healing

Layer / File(s)	Summary
Corrupt marker self-heal internal/sandbox/windows_unelevated.go, internal/sandbox/windows_unelevated_test.go	On JSON unmarshal failure, loadWindowsUnelevatedSetupMarker now returns an empty marker with the current schema version and nil error instead of a parse error; a new test verifies this self-healing and confirms a subsequently recorded applied plan persists correctly.

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

Sequence Diagram(s)

sequenceDiagram
  participant Runner
  participant loadWindowsUnelevatedSetupMarker
  participant MarkerFile

  Runner->>loadWindowsUnelevatedSetupMarker: load marker
  loadWindowsUnelevatedSetupMarker->>MarkerFile: read marker body
  MarkerFile-->>loadWindowsUnelevatedSetupMarker: corrupt/truncated JSON
  loadWindowsUnelevatedSetupMarker-->>Runner: empty marker (current SchemaVersion), nil error
  Runner->>loadWindowsUnelevatedSetupMarker: record new applied plan
  loadWindowsUnelevatedSetupMarker->>MarkerFile: rewrite marker

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: making the sandbox self-heal a corrupt unelevated setup marker.

✨ 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

• X
• Mastodon
• Reddit
• LinkedIn

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

[coderabbitai]

🧹 Nitpick comments (1)

internal/sandbox/windows_unelevated.go (1)

81-84: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider a debug-level log on corrupt-marker resets for observability.

Swallowing the unmarshal error entirely means there's no trace if the marker file gets corrupted repeatedly (e.g., concurrent writers, disk issues). A low-level log line here would help diagnose recurring corruption without affecting the self-heal behavior.

💡 Optional observability addition

 	var marker WindowsUnelevatedSetupMarker
 	if err := json.Unmarshal(bytes, &marker); err != nil {
+		// Note: intentionally not returning err; corrupt bodies self-heal like unknown schemas.
 		return WindowsUnelevatedSetupMarker{SchemaVersion: windowsUnelevatedSetupMarkerSchemaVersion}, nil
 	}

🤖 Prompt for 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.

In `@internal/sandbox/windows_unelevated.go` around lines 81 - 84, The marker
unmarshal path in WindowsUnelevatedSetupMarker loading is silently self-healing
on corrupt data, but it should emit a debug-level log when json.Unmarshal fails
so repeated corruption can be diagnosed. Update the error handling around the
WindowsUnelevatedSetupMarker decode to keep returning the default marker while
logging the corruption/reset event with enough context, using the existing
logger path in this codebase if available.

🤖 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.

Nitpick comments:
In `@internal/sandbox/windows_unelevated.go`:
- Around line 81-84: The marker unmarshal path in WindowsUnelevatedSetupMarker
loading is silently self-healing on corrupt data, but it should emit a
debug-level log when json.Unmarshal fails so repeated corruption can be
diagnosed. Update the error handling around the WindowsUnelevatedSetupMarker
decode to keep returning the default marker while logging the corruption/reset
event with enough context, using the existing logger path in this codebase if
available.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: b29474db-ef45-449c-9fd9-fc58dc914ae6

📥 Commits

Reviewing files that changed from the base of the PR and between b63fe69 and d036bea.

📒 Files selected for processing (2)

• internal/sandbox/windows_unelevated.go
• internal/sandbox/windows_unelevated_test.go

[kevincodex1]

lgtm . thanks!

[Vasanthdev2004]

Approve — solid robustness fix, correct and safe

Worth it: yes. Before this, loadWindowsUnelevatedSetupMarker hard-errored on a corrupt or truncated windows-unelevated-setup.json, which bricked every unelevated sandbox command until the file was hand-deleted — a realistic failure mode (crash / power-loss mid-write, disk issue). The fix makes a corrupt body self-heal exactly like the already-handled unknown-schema case: reset to an empty current-schema marker and let the runner re-apply and rewrite. Good follow-up to the #427 review feedback. Thanks @euxaristia.

Why it's correct and safe (checked each):

• No enforcement skip. The marker only memoizes idempotent ACL applies. Resetting forces a redundant re-apply next command — the fail-safe direction. It can never cause an ACL grant to be skipped.
• No torn-write race. The healed marker is written through the existing temp-file-then-os.Rename atomic path, so readers never see a partial write.
• Doesn't swallow real errors. Only json.Unmarshal failures convert to a reset; genuine read/permission/IO errors still propagate. And since Go ignores unknown JSON fields, a forward-compatible future schema still parses and routes through the separate SchemaVersion check rather than this path.

Verified on Windows 11: go build ./internal/sandbox/ + go vet clean; go test ./internal/sandbox/ green including the new TestLoadWindowsUnelevatedSetupMarkerSelfHealsCorruptFile (covers both halves: truncated body loads as empty without error, and a subsequent record over the corrupt file succeeds). No hardcoded-string test break. CI 7/7 green including Smoke (windows-latest).

Tight fix with a clear rationale in the doc comment. Approving.