fix: address review feedback for clientinject engine preflight/backup

Windows preflight always CreateFile share-0; never clobber stock
.openfsd-bak on re-Apply; AbsPrimaryPE on Plan/Apply; re-verify +
stock SHA on first Apply; health-fail clears MutationsApplied;
in_progress refuses re-Apply; AGENTS/doc/nits.

AGENTS.md

@@ -32,6 +32,7 @@ Operational rules for agents and humans changing this repository. This file is t
 | `internal/clientinject` | Engine plan/apply/revert | Must not import server/web/afv/db/postoffice/session/cluster/sweatbox/metar/auth/serviceapi |
 | `internal/clientinject/cilus` | Pure CLR `#US` encode/decode | **stdlib only** |
 | `internal/clientinject/vpilotconfig` | Pure vPilot 3DES config crypto + XML | **stdlib only** |
+| `internal/clientinject/pepatch` | PE/binary overwrite + padded string + UTF-16 scan | **stdlib only** |
 
 **Wire format:** `pkg/protocol` (FSD) and `pkg/twrfiles` (.apt/.air) — no alternate field order or marshaling in handlers/clients.
 
@@ -60,7 +61,8 @@ internal/auth     → protocol
 internal/session  → protocol
 internal/clientinject/cilus → (stdlib only)
 internal/clientinject/vpilotconfig → (stdlib only)
-internal/clientinject → cilus, vpilotconfig, …; never server/web/afv/db/…
+internal/clientinject/pepatch → (stdlib only)
+internal/clientinject → cilus, vpilotconfig, pepatch, …; never server/web/afv/db/…
 cmd/openfsd-client → internal/clientinject (+ GUI); never server/web/afv/db/…
 ```
 
@@ -88,8 +90,9 @@ Enforce with `scripts/check-import-graph.sh`.
 | `internal/clientinject` (incl. subpackages) | `server`, `web`, `afv`, `db`, `postoffice`, `session`, `cluster`, `sweatbox`, `metar`, `auth`, `serviceapi` |
 | `internal/clientinject/cilus` | Any non-stdlib import |
 | `internal/clientinject/vpilotconfig` | Any non-stdlib import |
+| `internal/clientinject/pepatch` | Any non-stdlib import |
 
-Stdlib heuristic: first path element contains no `.` (e.g. `fmt`, `net/http`). Third-party is never allowed in `pkg/protocol`, `pkg/twrfiles`, `internal/geo`, `internal/clientinject/cilus`, or `internal/clientinject/vpilotconfig`.
+Stdlib heuristic: first path element contains no `.` (e.g. `fmt`, `net/http`). Third-party is never allowed in `pkg/protocol`, `pkg/twrfiles`, `internal/geo`, `internal/clientinject/cilus`, `internal/clientinject/vpilotconfig`, or `internal/clientinject/pepatch`.
 
 **Cycle rule:** `session` never imports `postoffice`. Postoffice depends on a narrow participant/send port. Shared errors like `ErrCallsignInUse` live next to the registry, not in `pkg/protocol`.
 
@@ -191,6 +194,8 @@ Enforced by `scripts/check-coverage.sh` (CI).
 | `internal/cluster` | ≥90% | **Hard** |
 | `internal/clientinject/cilus` | ≥98% | **Hard** |
 | `internal/clientinject/vpilotconfig` | ≥98% | **Hard** |
+| `internal/clientinject/pepatch` | ≥95% | **Hard** |
+| `internal/clientinject` | ≥85% | Soft (report only; hard after GUI/adapters mature) |
 | `internal/web` | ≥80% | Soft (report only) |
 | `internal/afv` | ≥80% | Soft (P0; hard ≥85 later) |
 | Overall aspirational | 90% | Soft (report only) |

internal/clientinject/backup.go

@@ -2,6 +2,7 @@ package clientinject
 
 import (
 	"encoding/json"
+	"errors"
 	"fmt"
 	"path/filepath"
 	"time"
@@ -21,6 +22,12 @@ const (
 	ManifestStatusReverted   = "reverted"
 )
 
+// ErrPriorInjectActive is returned when Apply finds an unfinished or applied
+// inject that must be Reverted first (or when bak would be clobbered).
+// For status=applied, re-Apply is allowed if stock bak is preserved; this
+// error is used for status=in_progress (incomplete prior Apply).
+var ErrPriorInjectActive = errors.New("clientinject: prior inject in progress; Revert first, then Apply again")
+
 // Manifest records a transactional Apply for Revert and recovery.
 type Manifest struct {
 	ClientID            string         `json:"client_id"`
@@ -102,7 +109,9 @@ func CollectPlanTargets(plan *Plan) []string {
 	return out
 }
 
-// CreateBackups copies each existing target to its .openfsd-bak sibling.
+// CreateBackups ensures each existing target has a .openfsd-bak sibling.
+// Existing bak files are never overwritten — they hold stock content from the
+// first Apply and must remain restorable across re-Apply.
 // Missing targets are skipped (they may be created by Apply).
 func CreateBackups(w FileWriter, targets []string) ([]ManifestFile, error) {
 	var files []ManifestFile
@@ -112,6 +121,11 @@ func CreateBackups(w FileWriter, targets []string) ([]ManifestFile, error) {
 			continue
 		}
 		bak := BackupPath(orig)
+		if _, err := w.Stat(bak); err == nil {
+			// Preserve existing stock bak.
+			files = append(files, ManifestFile{Original: orig, Backup: bak})
+			continue
+		}
 		if err := w.CopyFile(orig, bak); err != nil {
 			return files, fmt.Errorf("clientinject: backup %s: %w", orig, err)
 		}
@@ -208,3 +222,22 @@ func NewManifest(plan *Plan, files []ManifestFile, preflightOK bool) *Manifest {
 		InstallRoot:         plan.Install.RootDir,
 	}
 }
+
+// CheckPriorInject refuses Apply when a prior inject is incomplete (in_progress).
+// status=applied / failed / reverted is OK: CreateBackups preserves stock bak.
+// Recovery: Revert uses bak even if status is still in_progress.
+func CheckPriorInject(w FileWriter, installRoot string) error {
+	path := ManifestPath(installRoot)
+	if _, err := w.Stat(path); err != nil {
+		return nil // no manifest
+	}
+	m, err := ReadManifest(w, installRoot)
+	if err != nil {
+		return fmt.Errorf("clientinject: read prior manifest: %w", err)
+	}
+	if m.Status == ManifestStatusInProgress {
+		return fmt.Errorf("%w (status=%s); Revert restores .openfsd-bak even if status is in_progress",
+			ErrPriorInjectActive, m.Status)
+	}
+	return nil
+}

internal/clientinject/doc.go

@@ -4,7 +4,8 @@
 //
 // Pure subpackages (stdlib only):
 //   - pepatch — PE/binary overwrite helpers
-//   - cilus / vpilotconfig — land in sibling PRs
+//   - cilus — CLR #US heap encode/decode
+//   - vpilotconfig — vPilot 3DES config crypto + XML field rewrite
 //
 // This package must not import server, web, afv, db, postoffice, session,
 // cluster, sweatbox, metar, auth, or serviceapi.

internal/clientinject/engine.go

@@ -87,9 +87,18 @@ func fileSHA1(w FileWriter, path string) (string, error) {
 	return hex.EncodeToString(sum[:]), nil
 }
 
+// normalizeInstall makes PrimaryPE absolute under RootDir when relative.
+func normalizeInstall(install Install) Install {
+	if pe := AbsPrimaryPE(install); pe != "" {
+		install.PrimaryPE = pe
+	}
+	return install
+}
+
 // Plan runs adapter.Plan after Verify. Does not write disk.
 func (e *Engine) Plan(ctx context.Context, install Install, ep Endpoints) (*Plan, error) {
 	_ = ctx
+	install = normalizeInstall(install)
 	a, ok := e.Adapters[install.ClientID]
 	if !ok {
 		return nil, fmt.Errorf("clientinject: no adapter for client %q", install.ClientID)
@@ -124,6 +133,13 @@ func (e *Engine) Plan(ctx context.Context, install Install, ep Endpoints) (*Plan
 
 // Apply runs preflight, plan blockers check, backup, adapter.Apply, healthcheck
 // with automatic restore on failure.
+//
+// Preflight is a probe only (lock released before mutations). PrimaryPE is
+// normalized via AbsPrimaryPE. Existing .openfsd-bak files are never clobbered
+// so re-Apply keeps stock for Revert. Incomplete prior inject (manifest
+// status=in_progress) must be Reverted first — Revert uses bak even if status
+// is still in_progress (e.g. finalize WriteManifest failed after a successful
+// patch).
 func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) {
 	if plan == nil {
 		return nil, fmt.Errorf("clientinject: nil plan")
@@ -131,18 +147,61 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) {
 	if len(plan.Blockers) > 0 {
 		return nil, fmt.Errorf("clientinject: plan has blockers: %s", strings.Join(plan.Blockers, "; "))
 	}
+	// Normalize PrimaryPE to absolute under RootDir for preflight + bak paths.
+	plan.Install = normalizeInstall(plan.Install)
+
 	a, ok := e.Adapters[plan.Install.ClientID]
 	if !ok {
 		return nil, fmt.Errorf("clientinject: no adapter for client %q", plan.Install.ClientID)
 	}
 	w := e.writer()
 
-	// Preflight: exclusive open on primary PE.
-	preflightOK := true
+	// Refuse incomplete prior inject (status=in_progress). Applied/failed/reverted OK.
+	if err := CheckPriorInject(w, plan.Install.RootDir); err != nil {
+		return nil, err
+	}
+
+	// Re-resolve profile + Verify at Apply entry (Plan may be stale / caller-built).
+	profile, err := e.ResolveProfile(plan.Install)
+	if err != nil {
+		return nil, err
+	}
+	if plan.Install.ProfileID == "" {
+		plan.Install.ProfileID = profile.ProfileID
+	}
+	// On first apply (no stock bak for PE yet), PE must match profile stock SHA.
+	// Re-apply after a prior successful inject keeps stock in .openfsd-bak while
+	// the live PE may already be patched — skip live-hash stock check then.
+	if plan.Install.PrimaryPE != "" {
+		want := strings.ToLower(strings.TrimSpace(profile.PrimaryBinary.SHA1))
+		bak := BackupPath(plan.Install.PrimaryPE)
+		if _, bakErr := w.Stat(bak); bakErr != nil && want != "" {
+			sum, err := fileSHA1(w, plan.Install.PrimaryPE)
+			if err != nil {
+				return nil, fmt.Errorf("clientinject: hash PE for verify: %w", err)
+			}
+			if !strings.EqualFold(sum, want) {
+				return nil, fmt.Errorf("clientinject: PE sha1 %s does not match profile %s stock %s",
+					sum, profile.ProfileID, want)
+			}
+			plan.Install.HashSHA1 = sum
+		} else if plan.Install.HashSHA1 == "" {
+			if sum, err := fileSHA1(w, plan.Install.PrimaryPE); err == nil {
+				plan.Install.HashSHA1 = sum
+			}
+		}
+	}
+	if err := a.Verify(plan.Install, profile); err != nil {
+		return nil, fmt.Errorf("clientinject: verify: %w", err)
+	}
+
+	// Preflight: exclusive open probe on absolute primary PE (lock not held across Apply).
+	preflightOK := false
 	if plan.Install.PrimaryPE != "" {
 		if err := PreflightPrimaryPE(plan.Install.PrimaryPE); err != nil {
 			return nil, err
 		}
+		preflightOK = true
 	}
 
 	targets := CollectPlanTargets(plan)
@@ -152,8 +211,8 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) {
 	}
 	manifest := NewManifest(plan, files, preflightOK)
 	if err := WriteManifest(w, manifest); err != nil {
-		// Best-effort restore if we already wrote bak.
-		_ = RestoreBackups(w, files)
+		// Best-effort restore only for newly created baks is complex; leave baks.
+		// Disk originals unchanged at this point (adapter not yet called).
 		return nil, err
 	}
 
@@ -165,17 +224,19 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) {
 		if rerr := RestoreBackups(w, files); rerr != nil {
 			slog.Error("clientinject restore after apply failure also failed", "err", rerr)
 			manifest.Status = ManifestStatusFailed
+			manifest.MutationsApplied = nil
 			manifest.Error = fmt.Sprintf("apply: %v; restore: %v", applyErr, rerr)
 			_ = WriteManifest(w, manifest)
 			return nil, fmt.Errorf("clientinject: apply failed (%w) and restore failed (%v)", applyErr, rerr)
 		}
 		manifest.Status = ManifestStatusFailed
+		manifest.MutationsApplied = nil
 		manifest.Error = applyErr.Error()
 		_ = WriteManifest(w, manifest)
 		return nil, fmt.Errorf("clientinject: apply failed (restored from bak): %w", applyErr)
 	}
 
-	// Record applied mutation IDs.
+	// Record applied mutation IDs (cleared again if healthcheck reverts).
 	applied := make([]string, 0, len(plan.Mutations))
 	for _, m := range plan.Mutations {
 		if m.ID != "" {
@@ -194,7 +255,9 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) {
 			_ = WriteManifest(w, manifest)
 			return nil, fmt.Errorf("clientinject: healthcheck failed (%w) and restore failed (%v)", err, rerr)
 		}
+		// Disk is stock again — do not claim mutations applied.
 		manifest.Status = ManifestStatusFailed
+		manifest.MutationsApplied = nil
 		manifest.Error = "healthcheck: " + err.Error()
 		_ = WriteManifest(w, manifest)
 		return nil, fmt.Errorf("clientinject: healthcheck failed (restored from bak): %w", err)
@@ -203,7 +266,11 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) {
 	manifest.Status = ManifestStatusApplied
 	manifest.Error = ""
 	if err := WriteManifest(w, manifest); err != nil {
-		return nil, fmt.Errorf("clientinject: finalize manifest: %w", err)
+		// PE/config remain patched; bak still holds stock. Manifest may stay
+		// in_progress — next Apply is refused until Revert (uses bak).
+		slog.Error("clientinject finalize manifest failed; disk patched, bak stock intact — Revert to recover",
+			"err", err, "install_root", plan.Install.RootDir)
+		return nil, fmt.Errorf("clientinject: finalize manifest: %w (disk may be patched; Revert restores from .openfsd-bak)", err)
 	}
 
 	return &ApplyResult{
@@ -214,6 +281,8 @@ func (e *Engine) Apply(ctx context.Context, plan *Plan) (*ApplyResult, error) {
 }
 
 // Revert restores bak files from the install root manifest.
+// After restore, if the primary PE path is known and a profile SHA-1 is on the
+// manifest, a soft mismatch is logged (does not fail Revert).
 func (e *Engine) Revert(ctx context.Context, installRoot string) error {
 	_ = ctx
 	if installRoot == "" {
@@ -230,8 +299,45 @@ func (e *Engine) Revert(ctx context.Context, installRoot string) error {
 	if err := RestoreBackups(w, m.Files); err != nil {
 		return err
 	}
+
+	// Soft stock-SHA check when profile is available.
+	if e.Profiles != nil && m.ProfileID != "" {
+		if p, ok := e.Profiles.Get(m.ProfileID); ok {
+			want := strings.ToLower(strings.TrimSpace(p.PrimaryBinary.SHA1))
+			// Find primary PE among restored files (first matching relative name) or Absolute.
+			pePath := ""
+			if p.PrimaryBinary.RelativePath != "" {
+				cand := filepath.Join(installRoot, p.PrimaryBinary.RelativePath)
+				for _, f := range m.Files {
+					if filepath.Clean(f.Original) == filepath.Clean(cand) {
+						pePath = f.Original
+						break
+					}
+				}
+				if pePath == "" {
+					// Fallback: any original ending with relative path.
+					for _, f := range m.Files {
+						if strings.HasSuffix(filepath.Clean(f.Original), filepath.Clean(p.PrimaryBinary.RelativePath)) {
+							pePath = f.Original
+							break
+						}
+					}
+				}
+			}
+			if pePath != "" && want != "" {
+				if sum, err := fileSHA1(w, pePath); err == nil {
+					if !strings.EqualFold(sum, want) {
+						slog.Warn("clientinject revert: primary PE sha1 != profile stock after restore",
+							"path", pePath, "got", sum, "want", want, "profile_id", m.ProfileID)
+					}
+				}
+			}
+		}
+	}
+
 	m.Status = ManifestStatusReverted
 	m.Error = ""
+	m.MutationsApplied = nil
 	if err := WriteManifest(w, m); err != nil {
 		return err
 	}

internal/clientinject/engine_test.go

@@ -405,6 +405,282 @@ func TestCollectPlanTargets(t *testing.T) {
 	}
 }
 
+func TestEngine_ReApplyPreservesStockBak(t *testing.T) {
+	// Apply → re-Apply with different payload → Revert must restore original stock.
+	root, pe, payload, stock := setupFakeInstall(t)
+	store := NewProfileStore()
+	_ = store.Add(&Profile{
+		SchemaVersion: 2,
+		ProfileID:     "fake-1",
+		ClientID:      "fake",
+		PrimaryBinary: PrimaryBinarySpec{
+			RelativePath: "app.exe",
+			SHA1:         sha1hex([]byte("MZ-fake-pe")),
+		},
+	})
+	fake := &FakeAdapter{ID: "fake", NewData: []byte("PATCH-V1")}
+	eng := NewEngine(store, fake)
+	install := Install{
+		ClientID:  "fake",
+		RootDir:   root,
+		PrimaryPE: pe,
+		HashSHA1:  sha1hex([]byte("MZ-fake-pe")),
+		ProfileID: "fake-1",
+	}
+	plan, err := eng.Plan(context.Background(), install, Endpoints{WebBaseURL: "https://x.test"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if _, err := eng.Apply(context.Background(), plan); err != nil {
+		t.Fatal(err)
+	}
+	// Stock bak must still be original after first apply.
+	bak1, err := os.ReadFile(payload + BackupSuffix)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !bytes.Equal(bak1, stock) {
+		t.Fatalf("first bak=%q stock=%q", bak1, stock)
+	}
+	// Re-apply different content; bak must not be clobbered with PATCH-V1.
+	fake.NewData = []byte("PATCH-V2")
+	plan2, err := eng.Plan(context.Background(), install, Endpoints{WebBaseURL: "https://y.test"})
+	if err != nil {
+		t.Fatal(err)
+	}
+	// Live PE is still stock (fake only patches payload.bin), so Plan/Verify OK.
+	if _, err := eng.Apply(context.Background(), plan2); err != nil {
+		t.Fatal(err)
+	}
+	bak2, err := os.ReadFile(payload + BackupSuffix)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !bytes.Equal(bak2, stock) {
+		t.Fatalf("re-apply clobbered bak: got %q want stock %q", bak2, stock)
+	}
+	got, _ := os.ReadFile(payload)
+	if !bytes.Equal(got, []byte("PATCH-V2")) {
+		t.Fatalf("payload=%q", got)
+	}
+	if err := eng.Revert(context.Background(), root); err != nil {
+		t.Fatal(err)
+	}
+	got, _ = os.ReadFile(payload)
+	if !bytes.Equal(got, stock) {
+		t.Fatalf("after revert got %q want stock", got)
+	}
+}
+
+func TestEngine_RelativePrimaryPE(t *testing.T) {
+	root, _, payload, stock := setupFakeInstall(t)
+	store := NewProfileStore()
+	_ = store.Add(&Profile{
+		SchemaVersion: 2,
+		ProfileID:     "fake-1",
+		ClientID:      "fake",
+		PrimaryBinary: PrimaryBinarySpec{RelativePath: "app.exe", SHA1: sha1hex([]byte("MZ-fake-pe"))},
+	})
+	fake := &FakeAdapter{ID: "fake", NewData: []byte("REL-PATCH")}
+	eng := NewEngine(store, fake)
+	install := Install{
+		ClientID:  "fake",
+		RootDir:   root,
+		PrimaryPE: "app.exe", // relative
+		ProfileID: "fake-1",
+		HashSHA1:  sha1hex([]byte("MZ-fake-pe")),
+	}
+	plan, err := eng.Plan(context.Background(), install, Endpoints{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !filepath.IsAbs(plan.Install.PrimaryPE) {
+		t.Fatalf("Plan should abs PrimaryPE, got %q", plan.Install.PrimaryPE)
+	}
+	if _, err := eng.Apply(context.Background(), plan); err != nil {
+		t.Fatal(err)
+	}
+	got, _ := os.ReadFile(payload)
+	if !bytes.Equal(got, []byte("REL-PATCH")) {
+		t.Fatalf("%q", got)
+	}
+	// Revert still works
+	if err := eng.Revert(context.Background(), root); err != nil {
+		t.Fatal(err)
+	}
+	got, _ = os.ReadFile(payload)
+	if !bytes.Equal(got, stock) {
+		t.Fatalf("revert %q", got)
+	}
+}
+
+func TestEngine_PriorInjectInProgressRefuses(t *testing.T) {
+	root, pe, _, _ := setupFakeInstall(t)
+	store := NewProfileStore()
+	_ = store.Add(&Profile{
+		SchemaVersion: 2,
+		ProfileID:     "fake-1",
+		ClientID:      "fake",
+		PrimaryBinary: PrimaryBinarySpec{RelativePath: "app.exe", SHA1: sha1hex([]byte("MZ-fake-pe"))},
+	})
+	// Stale in_progress manifest (e.g. finalize WriteManifest failed after patch).
+	m := &Manifest{
+		InstallRoot: root,
+		Status:      ManifestStatusInProgress,
+		ClientID:    "fake",
+		ProfileID:   "fake-1",
+		Files:       []ManifestFile{{Original: pe, Backup: BackupPath(pe)}},
+	}
+	if err := WriteManifest(OSFileWriter{}, m); err != nil {
+		t.Fatal(err)
+	}
+	fake := &FakeAdapter{ID: "fake"}
+	eng := NewEngine(store, fake)
+	plan := &Plan{
+		Install:   Install{ClientID: "fake", RootDir: root, PrimaryPE: pe, ProfileID: "fake-1", HashSHA1: sha1hex([]byte("MZ-fake-pe"))},
+		Mutations: []Mutation{{ID: "x", TargetRel: "payload.bin", Detail: WriteFileDetail{Contents: []byte("x")}}},
+	}
+	_, err := eng.Apply(context.Background(), plan)
+	if err == nil || !errors.Is(err, ErrPriorInjectActive) {
+		t.Fatalf("err=%v", err)
+	}
+}
+
+func TestEngine_ApplyVerifyMismatch(t *testing.T) {
+	root, pe, _, _ := setupFakeInstall(t)
+	// Wrong PE contents vs profile stock SHA.
+	if err := os.WriteFile(pe, []byte("WRONG-PE"), 0o644); err != nil {
+		t.Fatal(err)
+	}
+	store := NewProfileStore()
+	_ = store.Add(&Profile{
+		SchemaVersion: 2,
+		ProfileID:     "fake-1",
+		ClientID:      "fake",
+		PrimaryBinary: PrimaryBinarySpec{RelativePath: "app.exe", SHA1: sha1hex([]byte("MZ-fake-pe"))},
+	})
+	fake := &FakeAdapter{ID: "fake"}
+	eng := NewEngine(store, fake)
+	plan := &Plan{
+		Install:   Install{ClientID: "fake", RootDir: root, PrimaryPE: pe, ProfileID: "fake-1"},
+		Mutations: []Mutation{{ID: "x", TargetRel: "payload.bin", Detail: WriteFileDetail{Contents: []byte("x")}}},
+	}
+	_, err := eng.Apply(context.Background(), plan)
+	if err == nil {
+		t.Fatal("expected sha mismatch")
+	}
+}
+
+func TestEngine_HealthFailClearsMutationsApplied(t *testing.T) {
+	root, pe, _, _ := setupFakeInstall(t)
+	store := NewProfileStore()
+	_ = store.Add(&Profile{
+		SchemaVersion: 2,
+		ProfileID:     "fake-1",
+		ClientID:      "fake",
+		PrimaryBinary: PrimaryBinarySpec{RelativePath: "app.exe", SHA1: sha1hex([]byte("MZ-fake-pe"))},
+	})
+	fake := &FakeAdapter{ID: "fake", NewData: []byte("BAD"), FailHealth: true}
+	eng := NewEngine(store, fake)
+	install := Install{ClientID: "fake", RootDir: root, PrimaryPE: pe, ProfileID: "fake-1", HashSHA1: sha1hex([]byte("MZ-fake-pe"))}
+	plan, err := eng.Plan(context.Background(), install, Endpoints{})
+	if err != nil {
+		t.Fatal(err)
+	}
+	_, err = eng.Apply(context.Background(), plan)
+	if err == nil {
+		t.Fatal("expected health error")
+	}
+	m, err := ReadManifest(OSFileWriter{}, root)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(m.MutationsApplied) != 0 {
+		t.Fatalf("MutationsApplied should be cleared after health restore, got %v", m.MutationsApplied)
+	}
+}
+
+func TestEngine_PreflightOKFalseWhenNoPE(t *testing.T) {
+	root := t.TempDir()
+	payload := filepath.Join(root, "payload.bin")
+	stock := []byte("stock")
+	if err := os.WriteFile(payload, stock, 0o644); err != nil {
+		t.Fatal(err)
+	}
+	store := NewProfileStore()
+	_ = store.Add(&Profile{
+		SchemaVersion: 2,
+		ProfileID:     "fake-1",
+		ClientID:      "fake",
+		PrimaryBinary: PrimaryBinarySpec{RelativePath: "app.exe"}, // empty sha — no stock check
+	})
+	fake := &FakeAdapter{ID: "fake", NewData: []byte("P")}
+	eng := NewEngine(store, fake)
+	plan := &Plan{
+		Install:   Install{ClientID: "fake", RootDir: root, PrimaryPE: "", ProfileID: "fake-1"},
+		Mutations: []Mutation{{ID: "write_payload", TargetRel: "payload.bin", Detail: WriteFileDetail{Contents: []byte("P")}}},
+	}
+	res, err := eng.Apply(context.Background(), plan)
+	if err != nil {
+		t.Fatal(err)
+	}
+	_ = res
+	m, _ := ReadManifest(OSFileWriter{}, root)
+	if m.PreflightOK {
+		t.Fatal("preflight_ok should be false when PrimaryPE empty")
+	}
+}
+
+func TestCreateBackups_PreservesExisting(t *testing.T) {
+	w := OSFileWriter{}
+	dir := t.TempDir()
+	orig := filepath.Join(dir, "f.bin")
+	bak := BackupPath(orig)
+	if err := w.WriteFile(orig, []byte("LIVE")); err != nil {
+		t.Fatal(err)
+	}
+	if err := w.WriteFile(bak, []byte("STOCK")); err != nil {
+		t.Fatal(err)
+	}
+	files, err := CreateBackups(w, []string{orig})
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(files) != 1 {
+		t.Fatalf("%v", files)
+	}
+	got, _ := w.ReadFile(bak)
+	if string(got) != "STOCK" {
+		t.Fatalf("clobbered bak: %q", got)
+	}
+}
+
+func TestFileWriter_WriteFilePreservesMode(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "x.bin")
+	if err := os.WriteFile(path, []byte("a"), 0o755); err != nil {
+		t.Fatal(err)
+	}
+	w := OSFileWriter{}
+	if err := w.WriteFile(path, []byte("bb")); err != nil {
+		t.Fatal(err)
+	}
+	info, err := os.Stat(path)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if info.Mode().Perm()&0o111 == 0 {
+		// On some FS execute bit may not stick; at least we tried 0o755.
+		// Require that we didn't force only 0o644 if OS preserves.
+		t.Logf("mode=%v (execute may be unsupported on this FS)", info.Mode())
+	}
+	// Content updated
+	got, _ := os.ReadFile(path)
+	if string(got) != "bb" {
+		t.Fatalf("%q", got)
+	}
+}
+
 func TestEngine_RegisterAdapterAndWriterNil(t *testing.T) {
 	eng := &Engine{Profiles: NewProfileStore()}
 	fake := &FakeAdapter{ID: "fake"}

internal/clientinject/filewriter.go

@@ -36,8 +36,18 @@ func (OSFileWriter) ReadFile(path string) ([]byte, error) {
 }
 
 // WriteFile implements FileWriter.
+// When overwriting an existing file, preserves its permission bits (so PE
+// rewrites via full-file WriteFile keep the executable bit on Unix/Wine).
+// New files default to 0o644. Prefer OpenReadWrite / pepatch for PE mutations.
 func (OSFileWriter) WriteFile(path string, data []byte) error {
-	return os.WriteFile(path, data, 0o644)
+	mode := fs.FileMode(0o644)
+	if info, err := os.Stat(path); err == nil {
+		mode = info.Mode().Perm()
+		if mode == 0 {
+			mode = 0o644
+		}
+	}
+	return os.WriteFile(path, data, mode)
 }
 
 // CopyFile implements FileWriter.

internal/clientinject/pepatch/pepatch.go

@@ -3,6 +3,7 @@
 package pepatch
 
 import (
+	"bytes"
 	"encoding/binary"
 	"errors"
 	"fmt"
@@ -129,21 +130,9 @@ func ScanUTF16String(data []byte, s string) []int64 {
 	var hits []int64
 	// Byte-aligned scan; PE strings are 2-byte aligned but residual scan is exhaustive.
 	for i := 0; i+need <= len(data); i++ {
-		if bytesEqual(data[i:i+need], pat) {
+		if bytes.Equal(data[i:i+need], pat) {
 			hits = append(hits, int64(i))
 		}
 	}
 	return hits
 }
-
-func bytesEqual(a, b []byte) bool {
-	if len(a) != len(b) {
-		return false
-	}
-	for i := range a {
-		if a[i] != b[i] {
-			return false
-		}
-	}
-	return true
-}

internal/clientinject/pepatch/pepatch_test.go

@@ -246,18 +246,6 @@ func TestScanUTF16String_TooShort(t *testing.T) {
 	}
 }
 
-func TestBytesEqual(t *testing.T) {
-	if bytesEqual([]byte{1}, []byte{1, 2}) {
-		t.Fatal("len")
-	}
-	if !bytesEqual([]byte{1, 2}, []byte{1, 2}) {
-		t.Fatal("eq")
-	}
-	if bytesEqual([]byte{1, 2}, []byte{1, 3}) {
-		t.Fatal("ne")
-	}
-}
-
 func TestEncodePadded_UTF16Alias(t *testing.T) {
 	// via PaddedStringOverwrite encoding aliases
 	var buf seekBuffer

internal/clientinject/preflight.go

@@ -11,9 +11,13 @@ import (
 var ErrClientRunning = errors.New("clientinject: client appears to be running (file locked); quit the client completely, then Apply again")
 
 // PreflightPrimaryPE tries to open the primary PE exclusively (O_RDWR +
-// platform exclusive lock). On lock/sharing failures it returns
-// ErrClientRunning. Best-effort: process-list detection is optional and not
-// required for the gate.
+// platform exclusive lock / CreateFile share-mode 0 on Windows).
+// On lock/sharing failures it returns ErrClientRunning.
+//
+// This is a best-effort probe only: the exclusive lock is released before
+// return. Engine.Apply does not hold the lock across CreateBackups/adapter
+// mutations (TOCTOU is possible if the client starts mid-Apply). Process-list
+// detection is optional and not required for the gate.
 func PreflightPrimaryPE(path string) error {
 	if path == "" {
 		return fmt.Errorf("clientinject: preflight: empty primary PE path")

internal/clientinject/preflight_windows.go

@@ -9,11 +9,14 @@ import (
 	"unsafe"
 )
 
-// On Windows, a second CreateFile with share-mode 0 fails if the PE is mapped
-// or open without share. We re-open via CreateFileW with dwShareMode=0.
-// If that fails with sharing violation, treat as client running.
-// Holding a simple *os.File from OpenFile may still allow another OpenFile
-// (Go uses FILE_SHARE_READ|WRITE); exclusive CreateFile is the real gate.
+// On Windows, CreateFileW with dwShareMode=0 is the real "client running" gate:
+// a mapped/open PE typically fails exclusive open with ERROR_SHARING_VIOLATION
+// even when no byte-range locks exist. LockFileEx alone is insufficient because
+// running EXEs usually do not hold range locks — LockFileEx can succeed while
+// the client is still running.
+//
+// We always attempt CreateFile exclusive. LockFileEx is a secondary signal only
+// (returns ErrClientRunning when it reports a lock/sharing violation).
 
 var (
 	modkernel32      = syscall.NewLazyDLL("kernel32.dll")
@@ -36,7 +39,11 @@ const (
 )
 
 func tryExclusiveLock(f *os.File) error {
-	// Prefer LockFileEx exclusive non-blocking on the already-open handle.
+	// Primary gate: CreateFile with share mode 0 (always).
+	if err := tryCreateFileExclusive(f.Name()); err != nil {
+		return err
+	}
+	// Secondary: LockFileEx on the already-open Go handle (best-effort).
 	var ol syscall.Overlapped
 	r1, _, e1 := procLockFileEx.Call(
 		f.Fd(),
@@ -52,8 +59,9 @@ func tryExclusiveLock(f *os.File) error {
 			errno == syscall.Errno(_ERROR_SHARING_VIOLATION) {
 			return fmt.Errorf("%w: LockFileEx: %v", ErrClientRunning, errno)
 		}
-		// Fallback: try CreateFile with share mode 0.
-		return tryCreateFileExclusive(f.Name())
+		// Non-lock LockFileEx failure after CreateFile exclusive succeeded:
+		// treat as probe noise (CreateFile already proved exclusive open).
+		return nil
 	}
 	return nil
 }

scripts/check-client-profiles-sync.sh

@@ -54,10 +54,7 @@ for emb in "$EMBED"/*.yaml "$EMBED"/*.yml; do
     echo "  FAIL: $base client_id embed=$ec mirror=$mc"
     failed=1
   fi
-  # primary_binary.sha1 appears after primary_binary: — use simple grep of sha1 under file
-  esha="$(grep -E '^\s+sha1:' "$emb" 2>/dev/null | head -1 | sed -E 's/.*sha1:[[:space:]]*//' | tr -d '"' || true)"
-  msha="$(grep -E '^\s+sha1:' "$mir" 2>/dev/null | head -1 | sed -E 's/.*sha1:[[:space:]]*//' | tr -d '"' || true)"
-  # Prefer the primary_binary sha1 line that is not under installer (second sha1 often PE)
+  # Prefer the primary_binary sha1 (not installer sha1 under installer:)
   esha_pe="$(awk '/^primary_binary:/{p=1} p&&/sha1:/{print $2; exit}' "$emb" | tr -d '"' || true)"
   msha_pe="$(awk '/^primary_binary:/{p=1} p&&/sha1:/{print $2; exit}' "$mir" | tr -d '"' || true)"
   if [[ -n "$esha_pe" && -n "$msha_pe" && "$esha_pe" != "$msha_pe" ]]; then