From 8348fa3ac8190cdbfb0fa7502fc05f7ae7388474 Mon Sep 17 00:00:00 2001 From: proxyctl coordinator Date: Fri, 17 Jul 2026 21:25:43 +0800 Subject: [PATCH] fix: finish progressive Target detection --- .../03-progressive-startup-and-refresh.md | 28 +- internal/app/app.go | 300 ++++++++++++------ internal/app/progressive_detection_test.go | 170 +++++++++- internal/apt/apt.go | 52 ++- internal/apt/apt_test.go | 15 + internal/dockerclient/dockerclient.go | 44 ++- internal/dockerclient/dockerclient_test.go | 13 + internal/dockerdaemon/dockerdaemon.go | 35 +- internal/dockerdaemon/dockerdaemon_test.go | 18 ++ internal/gnome/gnome.go | 50 +-- internal/gnome/gnome_test.go | 19 ++ internal/shell/shell.go | 28 +- internal/shell/shell_test.go | 18 ++ internal/tui/dashboard.go | 8 +- internal/tui/model.go | 45 ++- internal/tui/pages.go | 2 +- internal/tui/progressive_detection_test.go | 48 ++- 17 files changed, 672 insertions(+), 221 deletions(-) diff --git a/.scratch/tui-redesign/issues/03-progressive-startup-and-refresh.md b/.scratch/tui-redesign/issues/03-progressive-startup-and-refresh.md index 69542cd..d7f2e2c 100644 --- a/.scratch/tui-redesign/issues/03-progressive-startup-and-refresh.md +++ b/.scratch/tui-redesign/issues/03-progressive-startup-and-refresh.md @@ -4,16 +4,20 @@ **Blocked by:** 02 — Deliver the Operations Console dashboard. -**Status:** ready-for-agent +**Status:** resolved -- [ ] Interactive application construction exposes Target identity and descriptions before availability and inspection complete. -- [ ] Startup renders every known Target in a structured Checking state without waiting for aggregate detection. -- [ ] Availability and inspection results stream to the TUI one Target at a time. -- [ ] Refresh uses the same progressive behavior rather than a separate aggregate-only path. -- [ ] Checking rows display a non-color-only progress cue and update independently on success or failure. -- [ ] Independent completion order does not reorder Targets or move selection to a different identity. -- [ ] A Target detection failure is represented truthfully without preventing unrelated Target results from appearing. -- [ ] The progressive Application Interface contains presentation-independent phases and results rather than Bubble Tea types or strings that callers must parse. -- [ ] Existing non-interactive maintenance behavior remains compatible. -- [ ] Application tests cover independent completion order, mixed success/failure, and final snapshots. -- [ ] Complete-Model tests prove the first useful frame precedes detection completion and each subsequent result updates only the relevant row. +- [x] Interactive application construction exposes Target identity and descriptions before availability and inspection complete. +- [x] Startup renders every known Target in a structured Checking state without waiting for aggregate detection. +- [x] Availability and inspection results stream to the TUI one Target at a time. +- [x] Refresh uses the same progressive behavior rather than a separate aggregate-only path. +- [x] Checking rows display a non-color-only progress cue and update independently on success or failure. +- [x] Independent completion order does not reorder Targets or move selection to a different identity. +- [x] A Target detection failure is represented truthfully without preventing unrelated Target results from appearing. +- [x] The progressive Application Interface contains presentation-independent phases and results rather than Bubble Tea types or strings that callers must parse. +- [x] Existing non-interactive maintenance behavior remains compatible. +- [x] Application tests cover independent completion order, mixed success/failure, and final snapshots. +- [x] Complete-Model tests prove the first useful frame precedes detection completion and each subsequent result updates only the relevant row. + +## Answer + +Progressive startup and refresh now publish authoritative Application snapshots per Target while generation checks discard stale overlapping results. Application state and snapshots share one lock, dispatch invalidates in-flight detection before committing mutations, and the dashboard retains selected Target identity—including a newly unavailable selected Target. Adapter detection failures are classified truthfully without changing maintenance inspection semantics. diff --git a/internal/app/app.go b/internal/app/app.go index bd79578..73f0bf2 100644 --- a/internal/app/app.go +++ b/internal/app/app.go @@ -53,17 +53,12 @@ type TargetSnapshot struct { Evidence string } -type TargetDetectionResult struct { - TargetID string - Phase TargetDetectionPhase - Availability target.Availability - Inspection target.Inspection - Err error -} - +// TargetDetectionUpdate carries the complete authoritative Application state +// after one Target changed. TargetID identifies that change without duplicating +// availability, inspection, or failure fields already present in Snapshot. type TargetDetectionUpdate struct { Snapshot Snapshot - Result TargetDetectionResult + TargetID string Complete bool } @@ -110,16 +105,20 @@ type ProgressiveDetectionModule interface { } type Application struct { - store profile.Store - state profile.State - targets []target.Target - availabilities map[string]target.Availability - inspections map[string]target.Inspection - detectionPhase map[string]TargetDetectionPhase - detectionErr map[string]error - detectionMu sync.RWMutex - mutationLock MutationLock - logger OperationLogger + store profile.Store + state profile.State + targets []target.Target + targetIDs []string + targetDescriptions []target.Description + availabilities map[string]target.Availability + inspections map[string]target.Inspection + detectionPhase map[string]TargetDetectionPhase + detectionErr map[string]error + detectionGeneration uint64 + stateMu sync.RWMutex + dispatchMu sync.Mutex + mutationLock MutationLock + logger OperationLogger } func New(store profile.Store) (*Application, error) { @@ -155,73 +154,87 @@ func NewWithTargetsAndRuntime(store profile.Store, targets []target.Target, muta logger: logger, } for _, adapter := range a.targets { - a.detectionPhase[adapter.ID()] = TargetChecking + id := adapter.ID() + a.targetIDs = append(a.targetIDs, id) + a.targetDescriptions = append(a.targetDescriptions, adapter.Description()) + a.detectionPhase[id] = TargetChecking } return a, nil } func (a *Application) Snapshot() Snapshot { - a.detectionMu.RLock() - defer a.detectionMu.RUnlock() + a.stateMu.RLock() + defer a.stateMu.RUnlock() return a.snapshotLocked() } func (a *Application) snapshotLocked() Snapshot { - profiles := append([]profile.Profile(nil), a.state.Profiles...) - snapshot := Snapshot{Profiles: profiles, ActiveProfile: a.state.ActiveProfile} - for _, adapter := range a.targets { - id := adapter.ID() + state := cloneState(a.state) + snapshot := Snapshot{Profiles: state.Profiles, ActiveProfile: state.ActiveProfile} + for i, id := range a.targetIDs { availability := a.availabilities[id] inspection := a.inspections[id] snapshot.Targets = append(snapshot.Targets, TargetSnapshot{ - ID: id, Description: adapter.Description(), DetectionPhase: a.detectionPhase[id], DetectionErr: a.detectionErr[id], + ID: id, Description: a.targetDescriptions[i], DetectionPhase: a.detectionPhase[id], DetectionErr: a.detectionErr[id], Available: availability.Available, AvailabilityEvidence: availability.Evidence, State: inspection.State, Evidence: inspection.Evidence, }) } return snapshot } +type targetDetectionOutcome struct { + targetID string + detection target.Detection +} + func (a *Application) BeginTargetDetection() (Snapshot, <-chan TargetDetectionUpdate) { - a.detectionMu.Lock() - for _, adapter := range a.targets { - id := adapter.ID() + a.stateMu.Lock() + a.detectionGeneration++ + generation := a.detectionGeneration + for _, id := range a.targetIDs { a.detectionPhase[id] = TargetChecking a.detectionErr[id] = nil } + active := activeProfile(a.state) checking := a.snapshotLocked() - a.detectionMu.Unlock() + a.stateMu.Unlock() updates := make(chan TargetDetectionUpdate, len(a.targets)+1) - active := a.activeProfile() - outcomes := make(chan TargetDetectionResult, len(a.targets)) - for _, adapter := range a.targets { - adapter := adapter + outcomes := make(chan targetDetectionOutcome, len(a.targets)) + for i, adapter := range a.targets { + adapter, id := adapter, a.targetIDs[i] go func() { - detection := detectTarget(adapter, active) - phase := TargetReady - if detection.Err != nil { - phase = TargetDetectionFailed - } - outcomes <- TargetDetectionResult{ - TargetID: adapter.ID(), Phase: phase, Availability: detection.Availability, - Inspection: detection.Inspection, Err: detection.Err, - } + outcomes <- targetDetectionOutcome{targetID: id, detection: detectTarget(adapter, active)} }() } go func() { defer close(updates) for range a.targets { - result := <-outcomes - a.detectionMu.Lock() - a.availabilities[result.TargetID] = result.Availability - a.inspections[result.TargetID] = result.Inspection - a.detectionPhase[result.TargetID] = result.Phase - a.detectionErr[result.TargetID] = result.Err - snapshot := a.snapshotLocked() - a.detectionMu.Unlock() - updates <- TargetDetectionUpdate{Snapshot: snapshot, Result: result} + outcome := <-outcomes + a.stateMu.Lock() + if generation != a.detectionGeneration { + a.stateMu.Unlock() + continue + } + phase := TargetReady + if outcome.detection.Err != nil { + phase = TargetDetectionFailed + } + a.availabilities[outcome.targetID] = outcome.detection.Availability + a.inspections[outcome.targetID] = outcome.detection.Inspection + a.detectionPhase[outcome.targetID] = phase + a.detectionErr[outcome.targetID] = outcome.detection.Err + update := TargetDetectionUpdate{Snapshot: a.snapshotLocked(), TargetID: outcome.targetID} + // The buffered send cannot wait on a consumer and happens before a + // newer Begin call can make this generation stale. + updates <- update + a.stateMu.Unlock() } - updates <- TargetDetectionUpdate{Snapshot: a.Snapshot(), Complete: true} + a.stateMu.Lock() + if generation == a.detectionGeneration { + updates <- TargetDetectionUpdate{Snapshot: a.snapshotLocked(), Complete: true} + } + a.stateMu.Unlock() }() return checking, updates } @@ -252,6 +265,11 @@ func (a *Application) DispatchProgress(intent Intent) <-chan Result { } func (a *Application) dispatch(intent Intent, publish func(Result)) Result { + // Serialize local mutation workflows even when tests intentionally omit the + // cross-process MutationLock. + a.dispatchMu.Lock() + defer a.dispatchMu.Unlock() + if a.mutationLock != nil { unlock, err := a.mutationLock.Lock(context.Background()) if err != nil { @@ -267,7 +285,7 @@ func (a *Application) dispatch(intent Intent, publish func(Result)) Result { if err != nil { return Result{Snapshot: a.Snapshot(), Err: fmt.Errorf("reload Profile state for mutation: %w", err)} } - a.state = fresh + a.replaceState(fresh) if intent.Kind == SetTargetDesired || intent.Kind == ActivateProfile { a.refreshInspections() } @@ -297,19 +315,19 @@ func (a *Application) dispatch(intent Intent, publish func(Result)) Result { return current, mutationErr }) if err == nil { - a.state = next + a.replaceState(next) a.refreshInspections() } else { var committed *profile.CommitError if errors.As(err, &committed) { // Rename committed next before directory durability reporting failed. // Prefer that state even if the required verification reload also fails. - a.state = next + a.replaceState(next) reloaded, loadErr := a.store.Load() if loadErr != nil { err = errors.Join(err, fmt.Errorf("reload committed Profile configuration: %w", loadErr)) } else { - a.state = reloaded + a.replaceState(reloaded) } } } @@ -317,22 +335,25 @@ func (a *Application) dispatch(intent Intent, publish func(Result)) Result { } func (a *Application) activateProfile(intent Intent, publish func(Result)) Result { - if index(a.state, intent.Name) < 0 { + state := a.stateCopy() + if index(state, intent.Name) < 0 { return Result{Snapshot: a.Snapshot(), Err: fmt.Errorf("Profile %q does not exist", intent.Name)} } - if a.state.ActiveProfile == intent.Name { + if state.ActiveProfile == intent.Name { return Result{Snapshot: a.Snapshot(), Notice: fmt.Sprintf("Profile %q is already Active", intent.Name)} } - var enabled []target.Target + inspections := a.inspectionsCopy() + var enabled []int var warnings []string - for _, adapter := range a.targets { - if a.inspections[adapter.ID()].State != target.Enabled { + for i, adapter := range a.targets { + id := a.targetIDs[i] + if inspections[id].State != target.Enabled { continue } - enabled = append(enabled, adapter) + enabled = append(enabled, i) if warning := adapter.Confirmation(true); warning != "" { - warnings = append(warnings, adapter.Description().Name+": "+warning) + warnings = append(warnings, a.targetDescriptions[i].Name+": "+warning) } } if len(warnings) > 0 && !intent.Confirm { @@ -346,28 +367,35 @@ func (a *Application) activateProfile(intent Intent, publish func(Result)) Resul if err != nil { var committed *profile.CommitError if errors.As(err, &committed) { - a.state = next + a.replaceState(next) if reloaded, loadErr := a.store.Load(); loadErr != nil { err = errors.Join(err, fmt.Errorf("reload committed Profile configuration: %w", loadErr)) } else { - a.state = reloaded + a.replaceState(reloaded) } } return Result{Snapshot: a.Snapshot(), Err: err} } - a.state = next + a.replaceState(next) active := a.activeProfile() // The Active Profile has changed before any Target write begins. Re-inspect - // immediately so rows not yet updated are truthfully Drifted rather than - // appearing Enabled against stale managed values. - for _, adapter := range a.targets { - a.inspections[adapter.ID()] = adapter.Inspect(context.Background(), active) + // without the state lock, then publish all observations atomically. + observed := make(map[string]target.Inspection, len(a.targets)) + for i, adapter := range a.targets { + observed[a.targetIDs[i]] = adapter.Inspect(context.Background(), active) } + a.replaceInspections(observed) + var progress []TargetProgress var failures []error - for _, adapter := range enabled { + updated := make(map[string]bool, len(enabled)) + for _, targetIndex := range enabled { + adapter := a.targets[targetIndex] + id := a.targetIDs[targetIndex] + name := a.targetDescriptions[targetIndex].Name + updated[id] = true if publish != nil { - publish(Result{Snapshot: a.Snapshot(), Progress: []TargetProgress{{TargetID: adapter.ID(), Applying: true, State: a.inspections[adapter.ID()].State}}, Notice: "Updating " + adapter.Description().Name + " Target"}) + publish(Result{Snapshot: a.Snapshot(), Progress: []TargetProgress{{TargetID: id, Applying: true, State: a.inspection(id).State}}, Notice: "Updating " + name + " Target"}) } operation := adapter.SetDesired(context.Background(), true, active) inspection := operation.Inspection @@ -378,34 +406,33 @@ func (a *Application) activateProfile(intent Intent, publish func(Result)) Resul inspection = adapter.Inspect(context.Background(), active) safeErr = fmt.Errorf("%s (fresh inspection: %s — %s)", profile.RedactText(operation.Err.Error()), inspection.State, profile.RedactText(inspection.Evidence)) } - a.inspections[adapter.ID()] = inspection - item := TargetProgress{TargetID: adapter.ID(), State: inspection.State, Evidence: inspection.Evidence, Err: safeErr} + a.replaceInspection(id, inspection) + item := TargetProgress{TargetID: id, State: inspection.State, Evidence: inspection.Evidence, Err: safeErr} progress = append(progress, item) if publish != nil { - publish(Result{Snapshot: a.Snapshot(), Progress: []TargetProgress{item}, Notice: adapter.Description().Name + " Target update completed", Err: safeErr}) + publish(Result{Snapshot: a.Snapshot(), Progress: []TargetProgress{item}, Notice: name + " Target update completed", Err: safeErr}) } if a.logger != nil { outcome, detail := "success", inspection.Evidence if safeErr != nil { outcome, detail = "failure", safeErr.Error() } - _ = a.logger.Write(LogEntry{Time: time.Now(), Operation: "switch-Active-Profile", Target: adapter.ID(), Result: outcome, Detail: RedactLogDetail(detail)}) + _ = a.logger.Write(LogEntry{Time: time.Now(), Operation: "switch-Active-Profile", Target: id, Result: outcome, Detail: RedactLogDetail(detail)}) } if safeErr != nil { - failures = append(failures, fmt.Errorf("%s Target: %w", adapter.Description().Name, safeErr)) + failures = append(failures, fmt.Errorf("%s Target: %w", name, safeErr)) } } // Re-inspect Targets which were not part of the transition so any managed // configuration is interpreted against the new Active Profile as well. - updated := make(map[string]bool, len(enabled)) - for _, adapter := range enabled { - updated[adapter.ID()] = true - } - for _, adapter := range a.targets { - if !updated[adapter.ID()] { - a.inspections[adapter.ID()] = adapter.Inspect(context.Background(), active) + remaining := make(map[string]target.Inspection) + for i, adapter := range a.targets { + id := a.targetIDs[i] + if !updated[id] { + remaining[id] = adapter.Inspect(context.Background(), active) } } + a.mergeInspections(remaining) result := Result{Snapshot: a.Snapshot(), Progress: progress, Notice: fmt.Sprintf("Activated Profile %q", intent.Name)} if len(failures) > 0 { result.Err = errors.Join(failures...) @@ -414,16 +441,80 @@ func (a *Application) activateProfile(intent Intent, publish func(Result)) Resul return result } -func (a *Application) activeProfile() *profile.Profile { - for i := range a.state.Profiles { - if a.state.Profiles[i].Name == a.state.ActiveProfile { - value := a.state.Profiles[i] +func activeProfile(state profile.State) *profile.Profile { + for i := range state.Profiles { + if state.Profiles[i].Name == state.ActiveProfile { + value := state.Profiles[i] + value.Bypass = append([]string(nil), value.Bypass...) return &value } } return nil } +func (a *Application) activeProfile() *profile.Profile { + a.stateMu.RLock() + defer a.stateMu.RUnlock() + return activeProfile(a.state) +} + +func (a *Application) stateCopy() profile.State { + a.stateMu.RLock() + defer a.stateMu.RUnlock() + return cloneState(a.state) +} + +func (a *Application) replaceState(state profile.State) { + a.stateMu.Lock() + defer a.stateMu.Unlock() + a.detectionGeneration++ + a.state = cloneState(state) +} + +func (a *Application) inspectionsCopy() map[string]target.Inspection { + a.stateMu.RLock() + defer a.stateMu.RUnlock() + result := make(map[string]target.Inspection, len(a.inspections)) + for id, inspection := range a.inspections { + result[id] = inspection + } + return result +} + +func (a *Application) inspection(id string) target.Inspection { + a.stateMu.RLock() + defer a.stateMu.RUnlock() + return a.inspections[id] +} + +func (a *Application) replaceInspection(id string, inspection target.Inspection) { + a.stateMu.Lock() + defer a.stateMu.Unlock() + a.detectionGeneration++ + a.inspections[id] = inspection +} + +func (a *Application) replaceInspections(inspections map[string]target.Inspection) { + a.stateMu.Lock() + defer a.stateMu.Unlock() + a.detectionGeneration++ + for _, id := range a.targetIDs { + a.inspections[id] = inspections[id] + } +} + +func (a *Application) mergeInspections(inspections map[string]target.Inspection) { + if len(inspections) == 0 { + return + } + a.stateMu.Lock() + defer a.stateMu.Unlock() + a.detectionGeneration++ + for id, inspection := range inspections { + a.inspections[id] = inspection + } +} + func (a *Application) refreshTargets() { _, updates := a.BeginTargetDetection() for range updates { @@ -432,25 +523,25 @@ func (a *Application) refreshTargets() { func (a *Application) refreshInspections() { active := a.activeProfile() - for _, adapter := range a.targets { - inspection := adapter.Inspect(context.Background(), active) - a.detectionMu.Lock() - a.inspections[adapter.ID()] = inspection - a.detectionMu.Unlock() + inspections := make(map[string]target.Inspection, len(a.targets)) + for i, adapter := range a.targets { + inspections[a.targetIDs[i]] = adapter.Inspect(context.Background(), active) } + a.replaceInspections(inspections) } func (a *Application) setTargetDesired(intent Intent) Result { - var selected target.Target - for _, adapter := range a.targets { - if adapter.ID() == intent.TargetID { - selected = adapter + selectedIndex := -1 + for i, id := range a.targetIDs { + if id == intent.TargetID { + selectedIndex = i break } } - if selected == nil { + if selectedIndex < 0 { return Result{Snapshot: a.Snapshot(), Err: fmt.Errorf("Target %q does not exist", intent.TargetID)} } + selected := a.targets[selectedIndex] active := a.activeProfile() if intent.Enabled && active == nil { return Result{Snapshot: a.Snapshot(), Err: errors.New("no Active Profile; open Profile management before enabling a Target")} @@ -463,7 +554,7 @@ func (a *Application) setTargetDesired(intent Intent) Result { if result.Err != nil { inspection = selected.Inspect(context.Background(), active) } - a.inspections[selected.ID()] = inspection + a.replaceInspection(intent.TargetID, inspection) outcome, detail := "success", inspection.Evidence if result.Err != nil { outcome, detail = "failure", result.Err.Error() @@ -473,11 +564,11 @@ func (a *Application) setTargetDesired(intent Intent) Result { if intent.Enabled { operation = "enable" } - _ = a.logger.Write(LogEntry{Time: time.Now(), Operation: operation, Target: selected.ID(), Result: outcome, Detail: RedactLogDetail(detail)}) + _ = a.logger.Write(LogEntry{Time: time.Now(), Operation: operation, Target: intent.TargetID, Result: outcome, Detail: RedactLogDetail(detail)}) } notice := "" if result.Err == nil { - notice = fmt.Sprintf("%s Target is now %s", selected.Description().Name, inspection.State) + notice = fmt.Sprintf("%s Target is now %s", a.targetDescriptions[selectedIndex].Name, inspection.State) } return Result{Snapshot: a.Snapshot(), Notice: notice, Err: result.Err} } @@ -551,5 +642,8 @@ func index(state profile.State, name string) int { func cloneState(state profile.State) profile.State { state.Profiles = append([]profile.Profile(nil), state.Profiles...) + for i := range state.Profiles { + state.Profiles[i].Bypass = append([]string(nil), state.Profiles[i].Bypass...) + } return state } diff --git a/internal/app/progressive_detection_test.go b/internal/app/progressive_detection_test.go index 7225b38..33efe43 100644 --- a/internal/app/progressive_detection_test.go +++ b/internal/app/progressive_detection_test.go @@ -3,8 +3,10 @@ package app_test import ( "context" "errors" + "fmt" "path/filepath" "reflect" + "sync" "testing" "proxyctl/internal/app" @@ -67,7 +69,7 @@ func TestApplicationStreamsIndependentTargetDetectionInProductOrder(t *testing.T close(secondRelease) second := <-updates - if second.Result.TargetID != "second" || second.Result.Phase != app.TargetReady || second.Result.Inspection.State != target.External { + if second.TargetID != "second" || second.Snapshot.Targets[1].DetectionPhase != app.TargetReady || second.Snapshot.Targets[1].State != target.External { t.Fatalf("second completion = %#v", second) } assertOnlyDetectionChanged(t, checking, second.Snapshot, "second") @@ -75,14 +77,14 @@ func TestApplicationStreamsIndependentTargetDetectionInProductOrder(t *testing.T close(failedRelease) failed := <-updates - if failed.Result.TargetID != "failed" || failed.Result.Phase != app.TargetDetectionFailed || failed.Result.Err == nil { + if failed.TargetID != "failed" || failed.Snapshot.Targets[2].DetectionPhase != app.TargetDetectionFailed || failed.Snapshot.Targets[2].DetectionErr == nil { t.Fatalf("failed completion = %#v", failed) } assertOnlyDetectionChanged(t, second.Snapshot, failed.Snapshot, "failed") close(firstRelease) first := <-updates - if first.Result.TargetID != "first" || first.Result.Phase != app.TargetReady || first.Result.Inspection.State != target.Enabled { + if first.TargetID != "first" || first.Snapshot.Targets[0].DetectionPhase != app.TargetReady || first.Snapshot.Targets[0].State != target.Enabled { t.Fatalf("first completion = %#v", first) } assertOnlyDetectionChanged(t, failed.Snapshot, first.Snapshot, "first") @@ -97,6 +99,168 @@ func TestApplicationStreamsIndependentTargetDetectionInProductOrder(t *testing.T } } +type detectionRequest struct { + result chan target.Detection +} + +type overlappingDetectionTarget struct { + requests chan detectionRequest +} + +func (t *overlappingDetectionTarget) ID() string { return "overlap" } +func (t *overlappingDetectionTarget) Description() target.Description { + return target.Description{Name: "Overlap", Summary: "controlled overlapping detection"} +} +func (t *overlappingDetectionTarget) Availability(context.Context) target.Availability { + return target.Availability{Available: true} +} +func (t *overlappingDetectionTarget) Inspect(context.Context, *profile.Profile) target.Inspection { + return target.Inspection{State: target.Disabled} +} +func (t *overlappingDetectionTarget) Confirmation(bool) string { return "" } +func (t *overlappingDetectionTarget) SetDesired(context.Context, bool, *profile.Profile) target.OperationResult { + return target.OperationResult{} +} +func (t *overlappingDetectionTarget) Detect(context.Context, *profile.Profile) target.Detection { + request := detectionRequest{result: make(chan target.Detection, 1)} + t.requests <- request + return <-request.result +} + +func TestOverlappingTargetDetectionDiscardsLateOlderGeneration(t *testing.T) { + adapter := &overlappingDetectionTarget{requests: make(chan detectionRequest, 2)} + application, err := app.NewWithTargets(profile.NewFileStore(filepath.Join(t.TempDir(), "profiles.json")), []target.Target{adapter}) + if err != nil { + t.Fatal(err) + } + + _, olderUpdates := application.BeginTargetDetection() + older := <-adapter.requests + _, newerUpdates := application.BeginTargetDetection() + newer := <-adapter.requests + + newer.result <- target.Detection{ + Availability: target.Availability{Available: true, Evidence: "new availability"}, + Inspection: target.Inspection{State: target.Enabled, Evidence: "new inspection"}, + } + for range newerUpdates { + } + older.result <- target.Detection{ + Availability: target.Availability{Evidence: "old availability"}, + Inspection: target.Inspection{State: target.Disabled, Evidence: "old inspection"}, + } + staleUpdates := 0 + for range olderUpdates { + staleUpdates++ + } + if staleUpdates != 0 { + t.Fatalf("older generation published %d stale authoritative update(s)", staleUpdates) + } + + got := application.Snapshot().Targets[0] + if !got.Available || got.State != target.Enabled || got.Evidence != "new inspection" { + t.Fatalf("late older generation overwrote newer result: %#v", got) + } +} + +type mutationInteractionTarget struct { + mu sync.Mutex + enabled bool + release chan struct{} +} + +func (t *mutationInteractionTarget) ID() string { return "mutation" } +func (t *mutationInteractionTarget) Description() target.Description { + return target.Description{Name: "Mutation", Summary: "detection and mutation interaction"} +} +func (t *mutationInteractionTarget) Availability(context.Context) target.Availability { + return target.Availability{Available: true} +} +func (t *mutationInteractionTarget) Inspect(context.Context, *profile.Profile) target.Inspection { + t.mu.Lock() + defer t.mu.Unlock() + state := target.Disabled + if t.enabled { + state = target.Enabled + } + return target.Inspection{State: state, Evidence: "current mutation state"} +} +func (t *mutationInteractionTarget) Confirmation(bool) string { return "" } +func (t *mutationInteractionTarget) SetDesired(context.Context, bool, *profile.Profile) target.OperationResult { + t.mu.Lock() + t.enabled = true + t.mu.Unlock() + return target.OperationResult{Inspection: target.Inspection{State: target.Enabled, Evidence: "mutation completed"}} +} +func (t *mutationInteractionTarget) Detect(context.Context, *profile.Profile) target.Detection { + <-t.release + return target.Detection{ + Availability: target.Availability{Available: true, Evidence: "stale detection"}, + Inspection: target.Inspection{State: target.Disabled, Evidence: "stale detection"}, + } +} + +func TestDispatchMutationInvalidatesInFlightTargetDetection(t *testing.T) { + adapter := &mutationInteractionTarget{release: make(chan struct{})} + application, err := app.NewWithTargets(profile.NewFileStore(filepath.Join(t.TempDir(), "profiles.json")), []target.Target{adapter}) + if err != nil { + t.Fatal(err) + } + created := application.Dispatch(app.Intent{Kind: app.CreateProfile, Profile: profile.Profile{Name: "office", SingleURL: "http://proxy.example:8080"}}) + if created.Err != nil { + t.Fatal(created.Err) + } + if activated := application.Dispatch(app.Intent{Kind: app.ActivateProfile, Name: "office"}); activated.Err != nil { + t.Fatal(activated.Err) + } + + _, updates := application.BeginTargetDetection() + result := application.Dispatch(app.Intent{Kind: app.SetTargetDesired, TargetID: adapter.ID(), Enabled: true}) + if result.Err != nil || result.Snapshot.Targets[0].State != target.Enabled { + t.Fatalf("mutation result = %#v", result) + } + close(adapter.release) + for range updates { + } + if got := application.Snapshot().Targets[0]; got.State != target.Enabled || got.Evidence != "mutation completed" { + t.Fatalf("in-flight detection overwrote mutation: %#v", got) + } +} + +func TestSnapshotIsSafeDuringConcurrentStateMutation(t *testing.T) { + application, err := app.New(profile.NewFileStore(filepath.Join(t.TempDir(), "profiles.json"))) + if err != nil { + t.Fatal(err) + } + + const count = 40 + start := make(chan struct{}) + var readers sync.WaitGroup + for range 4 { + readers.Add(1) + go func() { + defer readers.Done() + <-start + for range count * 4 { + _ = application.Snapshot() + } + }() + } + close(start) + for i := range count { + result := application.Dispatch(app.Intent{Kind: app.CreateProfile, Profile: profile.Profile{ + Name: fmt.Sprintf("profile-%02d", i), SingleURL: "http://proxy.example:8080", + }}) + if result.Err != nil { + t.Fatal(result.Err) + } + } + readers.Wait() + if got := len(application.Snapshot().Profiles); got != count { + t.Fatalf("Profile count = %d, want %d", got, count) + } +} + func assertTargetOrder(t *testing.T, snapshot app.Snapshot, want []string) { t.Helper() got := make([]string, len(snapshot.Targets)) diff --git a/internal/apt/apt.go b/internal/apt/apt.go index c487d30..cb68af5 100644 --- a/internal/apt/apt.go +++ b/internal/apt/apt.go @@ -155,32 +155,54 @@ func (a *Adapter) DiagnosticChecks(ctx context.Context, _ *profile.Profile) []ta } func (a *Adapter) Availability(ctx context.Context) target.Availability { - availability, _ := a.detectAvailability(ctx) + availability, _, _ := a.detectAvailability(ctx) return availability } -func (a *Adapter) detectAvailability(ctx context.Context) (target.Availability, error) { +// detectAvailability returns the effective dump used to prove capability so a +// progressive Detect can classify state without running apt-config twice. +func (a *Adapter) detectAvailability(ctx context.Context) (target.Availability, []byte, error) { marker, err := os.Stat(a.configuration.DebianVersionFile) - if err != nil || !marker.Mode().IsRegular() { - return target.Availability{Evidence: "Debian-family marker /etc/debian_version was not detected"}, nil + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return target.Availability{Evidence: "Debian-family marker /etc/debian_version was not detected"}, nil, nil + } + return target.Availability{Evidence: "Debian-family marker /etc/debian_version could not be inspected"}, nil, fmt.Errorf("inspect Debian-family marker: %w", err) + } + if !marker.Mode().IsRegular() { + return target.Availability{Evidence: "Debian-family marker /etc/debian_version was not detected"}, nil, nil } directory, err := os.Stat(a.configuration.ConfigDirectory) - if err != nil || !directory.IsDir() { - return target.Availability{Evidence: "APT configuration directory apt.conf.d was not detected"}, nil + if err != nil { + if errors.Is(err, fs.ErrNotExist) { + return target.Availability{Evidence: "APT configuration directory apt.conf.d was not detected"}, nil, nil + } + return target.Availability{Evidence: "APT configuration directory apt.conf.d could not be inspected"}, nil, fmt.Errorf("inspect APT configuration directory: %w", err) + } + if !directory.IsDir() { + return target.Availability{Evidence: "APT configuration directory apt.conf.d was not detected"}, nil, nil } command, err := a.runner.LookPath("apt-config") if err != nil { - return target.Availability{Evidence: "Debian APT tool apt-config was not detected"}, nil + if errors.Is(err, fs.ErrNotExist) || errors.Is(err, exec.ErrNotFound) { + return target.Availability{Evidence: "Debian APT tool apt-config was not detected"}, nil, nil + } + return target.Availability{Evidence: "Debian APT tool apt-config could not be inspected"}, nil, fmt.Errorf("locate apt-config: %w", err) } - if _, err := a.runner.Output(ctx, command, "dump"); err != nil { - return target.Availability{Evidence: "apt-config could not inspect effective APT configuration"}, fmt.Errorf("inspect effective APT configuration: %w", err) + effective, err := a.runner.Output(ctx, command, "dump") + if err != nil { + return target.Availability{Evidence: "apt-config could not inspect effective APT configuration"}, nil, fmt.Errorf("inspect effective APT configuration: %w", err) } - return target.Availability{Available: true, Evidence: "Debian marker, apt-config, and apt.conf.d configuration capability detected"}, nil + return target.Availability{Available: true, Evidence: "Debian marker, apt-config, and apt.conf.d configuration capability detected"}, effective, nil } func (a *Adapter) Detect(ctx context.Context, active *profile.Profile) target.Detection { - availability, availabilityErr := a.detectAvailability(ctx) - inspection, inspectionErr := a.inspect(ctx, active) + availability, effective, availabilityErr := a.detectAvailability(ctx) + if !availability.Available && availabilityErr == nil { + return target.Detection{Availability: availability} + } + overlay, overlayErr := a.files.ReadFile(a.configuration.OverlayFile) + inspection, inspectionErr := a.inspectObserved(active, overlay, overlayErr, effective, availabilityErr) return target.Detection{Availability: availability, Inspection: inspection, Err: errors.Join(availabilityErr, inspectionErr)} } @@ -193,11 +215,15 @@ func (a *Adapter) Inspect(ctx context.Context, active *profile.Profile) target.I func (a *Adapter) inspect(ctx context.Context, active *profile.Profile) (target.Inspection, error) { overlay, overlayErr := a.files.ReadFile(a.configuration.OverlayFile) + effective, dumpErr := a.dump(ctx) + return a.inspectObserved(active, overlay, overlayErr, effective, dumpErr) +} + +func (a *Adapter) inspectObserved(active *profile.Profile, overlay []byte, overlayErr error, effective []byte, dumpErr error) (target.Inspection, error) { managed := overlayErr == nil && isOwnedOverlay(overlay) if overlayErr != nil && !errors.Is(overlayErr, fs.ErrNotExist) { return target.Inspection{State: target.Drifted, Evidence: "managed APT overlay cannot be read"}, fmt.Errorf("read managed APT overlay: %w", overlayErr) } - effective, dumpErr := a.dump(ctx) if dumpErr != nil { if managed { return target.Inspection{State: target.Drifted, Evidence: "managed APT overlay exists but effective configuration cannot be inspected"}, dumpErr diff --git a/internal/apt/apt_test.go b/internal/apt/apt_test.go index f16aa97..08e6b38 100644 --- a/internal/apt/apt_test.go +++ b/internal/apt/apt_test.go @@ -46,6 +46,19 @@ func TestAvailabilityRequiresDebianMarkerAPTToolAndConfigurationDirectory(t *tes } } +func TestDetectClassifiesInspectionFailureAndUsesOneEffectiveConfigurationProbe(t *testing.T) { + a, runner := newAdapter(t, nil) + runner.outputError = true + + detection := a.Detect(context.Background(), compatibleProfile()) + if detection.Err == nil || detection.Inspection.State != target.Disabled { + t.Fatalf("detection = %#v, want failed Disabled inspection", detection) + } + if runner.outputs != 1 { + t.Fatalf("Detect issued %d apt-config dumps, want one", runner.outputs) + } +} + func TestDiagnosticChecksCoverCommandSchemaAndAuthorityWithoutMutation(t *testing.T) { a, _ := newAdapter(t, nil) checks := a.DiagnosticChecks(context.Background(), compatibleProfile()) @@ -442,6 +455,7 @@ func newFailingAdapter(t *testing.T, files *failingFS) (*apt.Adapter, *fakeRunne type fakeRunner struct { available, outputError bool overlay, external, override, failDumpAfterWrite string + outputs int } func (r *fakeRunner) LookPath(string) (string, error) { @@ -451,6 +465,7 @@ func (r *fakeRunner) LookPath(string) (string, error) { return "/usr/bin/apt-config", nil } func (r *fakeRunner) Output(context.Context, string, ...string) ([]byte, error) { + r.outputs++ if r.outputError { return nil, errors.New("apt-config dump failed") } diff --git a/internal/dockerclient/dockerclient.go b/internal/dockerclient/dockerclient.go index b5ce78b..11560ce 100644 --- a/internal/dockerclient/dockerclient.go +++ b/internal/dockerclient/dockerclient.go @@ -235,29 +235,54 @@ func (a *Adapter) RecoveryPath() string { return a.configuration.RecoveryFile } func (a *Adapter) LockPath() string { return a.configuration.RecoveryFile + ".lock" } func (a *Adapter) Detect(ctx context.Context, active *profile.Profile) target.Detection { - return target.Detection{Availability: a.Availability(ctx), Inspection: a.Inspect(ctx, active)} + inspection, err := a.inspect(active) + return target.Detection{Availability: a.Availability(ctx), Inspection: inspection, Err: redactError(err, active)} } +// Inspect preserves the maintenance state projection while Detect also +// reports malformed or unreadable state as a failed check. func (a *Adapter) Inspect(_ context.Context, active *profile.Profile) target.Inspection { + inspection, _ := a.inspect(active) + return inspection +} + +func (a *Adapter) inspect(active *profile.Profile) (target.Inspection, error) { metadata, managed, metadataErr := a.readRecovery() config, _, configErr := a.readConfig() if managed || (metadataErr != nil && !errors.Is(metadataErr, fs.ErrNotExist)) { if metadataErr != nil { - return target.Inspection{State: target.Drifted, Evidence: "managed Docker client recovery metadata cannot be read"} + return target.Inspection{State: target.Drifted, Evidence: "managed Docker client recovery metadata cannot be read"}, fmt.Errorf("inspect Docker client recovery metadata: %w", metadataErr) } if configErr != nil { - return target.Inspection{State: target.Drifted, Evidence: "managed Docker client configuration is missing, unreadable, or not safely mergeable"} + return target.Inspection{State: target.Drifted, Evidence: "managed Docker client configuration is missing, unreadable, or not safely mergeable"}, fmt.Errorf("inspect managed Docker client configuration: %w", configErr) } desired, desiredErr := desiredDefaults(active) if desiredErr == nil && fingerprintCurrent(config) == metadata.Fingerprint && fingerprintDefaults(desired) == metadata.Fingerprint { - return target.Inspection{State: target.Enabled, Evidence: "managed Docker client proxy defaults match the Active Profile"} + return target.Inspection{State: target.Enabled, Evidence: "managed Docker client proxy defaults match the Active Profile"}, nil } - return target.Inspection{State: target.Drifted, Evidence: "managed Docker client proxy defaults do not match the Active Profile"} + return target.Inspection{State: target.Drifted, Evidence: "managed Docker client proxy defaults do not match the Active Profile"}, nil } - if configErr == nil && recognizesProxyDefaults(config) { - return target.Inspection{State: target.External, Evidence: "recognized proxy defaults exist in Docker client config.json"} + if configErr != nil { + return target.Inspection{State: target.Disabled, Evidence: "Docker client configuration could not be inspected safely"}, fmt.Errorf("inspect Docker client configuration: %w", configErr) } - return target.Inspection{State: target.Disabled, Evidence: "no recognized Docker client proxy defaults detected"} + proxies, _, err := childObject(config, "proxies") + if err != nil { + return target.Inspection{State: target.Disabled, Evidence: "Docker client configuration could not be inspected safely"}, err + } + defaults, _, err := childObject(proxies, "default") + if err != nil { + return target.Inspection{State: target.Disabled, Evidence: "Docker client configuration could not be inspected safely"}, err + } + if err := validateManagedProxyStrings(defaults); err != nil { + return target.Inspection{State: target.Disabled, Evidence: "Docker client configuration could not be inspected safely"}, err + } + for _, key := range append(append([]string(nil), managedKeys...), "ftpProxy") { + var value string + if raw, ok := defaults[key]; ok && json.Unmarshal(raw, &value) == nil && value != "" { + return target.Inspection{State: target.External, Evidence: "recognized proxy defaults exist in Docker client config.json"}, nil + } + } + return target.Inspection{State: target.Disabled, Evidence: "no recognized Docker client proxy defaults detected"}, nil } func (a *Adapter) SetDesired(ctx context.Context, enabled bool, active *profile.Profile) target.OperationResult { @@ -613,6 +638,9 @@ type redactedError struct { func (e *redactedError) Error() string { return e.message } func (e *redactedError) Is(other error) bool { return e.class != nil && errors.Is(e.class, other) } func redactError(err error, active *profile.Profile) error { + if err == nil { + return nil + } message := profile.RedactText(err.Error()) if active != nil { for _, raw := range []string{active.SingleURL, active.HTTPURL, active.HTTPSURL, active.AllURL} { diff --git a/internal/dockerclient/dockerclient_test.go b/internal/dockerclient/dockerclient_test.go index 5866403..5e92284 100644 --- a/internal/dockerclient/dockerclient_test.go +++ b/internal/dockerclient/dockerclient_test.go @@ -20,6 +20,19 @@ import ( "proxyctl/internal/target/targettest" ) +func TestDetectClassifiesMalformedClientConfigurationAsFailure(t *testing.T) { + a := newAdapter(t, nil) + writeConfig(t, a.ConfigPath(), `{"proxies":[]}`) + + detection := a.Detect(context.Background(), activeProfile()) + if detection.Err == nil || detection.Inspection.State != target.Disabled { + t.Fatalf("detection = %#v, want failed Disabled inspection", detection) + } + if got := a.Inspect(context.Background(), activeProfile()); got.State != target.Disabled { + t.Fatalf("maintenance inspection = %#v, want established Disabled semantics", got) + } +} + func TestAvailabilityAndDescriptionDescribeOnlyDockerClientDefaults(t *testing.T) { a := newAdapter(t, nil) availability := a.Availability(context.Background()) diff --git a/internal/dockerdaemon/dockerdaemon.go b/internal/dockerdaemon/dockerdaemon.go index cab860f..3d22478 100644 --- a/internal/dockerdaemon/dockerdaemon.go +++ b/internal/dockerdaemon/dockerdaemon.go @@ -184,8 +184,9 @@ func (a *Adapter) detect(ctx context.Context) (target.Availability, error) { } func (a *Adapter) Detect(ctx context.Context, active *profile.Profile) target.Detection { - availability, err := a.detect(ctx) - return target.Detection{Availability: availability, Inspection: a.Inspect(ctx, active), Err: err} + availability, availabilityErr := a.detect(ctx) + inspection, inspectionErr := a.inspect(active) + return target.Detection{Availability: availability, Inspection: inspection, Err: errors.Join(availabilityErr, redact(inspectionErr, active))} } func parseProperties(output string) map[string]string { @@ -209,27 +210,43 @@ type recovery struct { } func (a *Adapter) Inspect(_ context.Context, active *profile.Profile) target.Inspection { + inspection, _ := a.inspect(active) + return inspection +} + +func (a *Adapter) inspect(active *profile.Profile) (target.Inspection, error) { metadata, managed, metadataErr := a.readRecovery() data, configErr := a.files.ReadFile(a.configuration.DaemonConfigPath) if managed || (metadataErr != nil && !errors.Is(metadataErr, fs.ErrNotExist)) { if metadataErr != nil { - return target.Inspection{State: target.Drifted, Evidence: "managed Docker daemon recovery metadata cannot be read"} + return target.Inspection{State: target.Drifted, Evidence: "managed Docker daemon recovery metadata cannot be read"}, fmt.Errorf("inspect Docker daemon recovery metadata: %w", metadataErr) } if configErr != nil { - return target.Inspection{State: target.Drifted, Evidence: "managed Docker daemon configuration is missing or unreadable"} + return target.Inspection{State: target.Drifted, Evidence: "managed Docker daemon configuration is missing or unreadable"}, fmt.Errorf("inspect managed Docker daemon configuration: %w", configErr) + } + if err := validateConfigSchema(data); err != nil { + return target.Inspection{State: target.Drifted, Evidence: "managed Docker daemon configuration is malformed"}, fmt.Errorf("inspect managed Docker daemon configuration: %w", err) } if fingerprint(data) == metadata.ConfigFingerprint && active != nil { normalized, err := compatible(*active) if err == nil && configMatchesProfile(data, normalized) { - return target.Inspection{State: target.Enabled, Evidence: "managed Docker daemon configuration matches the Active Profile"} + return target.Inspection{State: target.Enabled, Evidence: "managed Docker daemon configuration matches the Active Profile"}, nil } } - return target.Inspection{State: target.Drifted, Evidence: "managed Docker daemon configuration does not match the Active Profile"} + return target.Inspection{State: target.Drifted, Evidence: "managed Docker daemon configuration does not match the Active Profile"}, nil } - if configErr == nil && recognizesProxy(data) { - return target.Inspection{State: target.External, Evidence: "recognized proxy settings exist in Docker daemon configuration"} + if configErr != nil && !errors.Is(configErr, fs.ErrNotExist) { + return target.Inspection{State: target.Disabled, Evidence: "Docker daemon configuration could not be inspected"}, fmt.Errorf("inspect Docker daemon configuration: %w", configErr) } - return target.Inspection{State: target.Disabled, Evidence: "no recognized Docker daemon proxy configuration detected"} + if configErr == nil { + if err := validateConfigSchema(data); err != nil { + return target.Inspection{State: target.Disabled, Evidence: "Docker daemon configuration could not be inspected"}, fmt.Errorf("inspect Docker daemon configuration: %w", err) + } + if recognizesProxy(data) { + return target.Inspection{State: target.External, Evidence: "recognized proxy settings exist in Docker daemon configuration"}, nil + } + } + return target.Inspection{State: target.Disabled, Evidence: "no recognized Docker daemon proxy configuration detected"}, nil } func (a *Adapter) SetDesired(ctx context.Context, enabled bool, active *profile.Profile) target.OperationResult { diff --git a/internal/dockerdaemon/dockerdaemon_test.go b/internal/dockerdaemon/dockerdaemon_test.go index d318977..039a45d 100644 --- a/internal/dockerdaemon/dockerdaemon_test.go +++ b/internal/dockerdaemon/dockerdaemon_test.go @@ -219,6 +219,24 @@ func TestDiagnosticServiceControlAuthorityUsesInjectedEUIDWithoutMutation(t *tes } } +func TestDetectClassifiesMalformedDaemonConfigurationAsFailure(t *testing.T) { + a, _, daemonPath, _, _ := fixture(t) + if err := os.MkdirAll(filepath.Dir(daemonPath), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(daemonPath, []byte(`{"proxies":[]}`), 0o600); err != nil { + t.Fatal(err) + } + + detection := a.Detect(context.Background(), activeProfile()) + if detection.Err == nil || detection.Inspection.State != target.Disabled { + t.Fatalf("detection = %#v, want failed Disabled inspection", detection) + } + if got := a.Inspect(context.Background(), activeProfile()); got.State != target.Disabled { + t.Fatalf("maintenance inspection = %#v, want established Disabled semantics", got) + } +} + func TestDiagnosticsAndEnableRejectMalformedDaemonProxyTypesWithoutMutation(t *testing.T) { for _, document := range []string{ `[]`, diff --git a/internal/gnome/gnome.go b/internal/gnome/gnome.go index 8c1bf42..0883758 100644 --- a/internal/gnome/gnome.go +++ b/internal/gnome/gnome.go @@ -348,15 +348,18 @@ func (a *Adapter) DiagnosticChecks(ctx context.Context, _ *profile.Profile) []ta } func (a *Adapter) Availability(ctx context.Context) target.Availability { - availability, _ := a.detectAvailability(ctx) + availability, _, _ := a.detectAvailability(ctx) return availability } -func (a *Adapter) detectAvailability(ctx context.Context) (target.Availability, error) { +// detectAvailability returns the settings observation that established +// availability so Detect can inspect that exact observation without issuing a +// second twelve-command gsettings probe. +func (a *Adapter) detectAvailability(ctx context.Context) (target.Availability, settings, error) { output, err := a.run(ctx, "list-schemas") if err != nil { safeErr := redact(err) - return target.Availability{Evidence: fmt.Sprintf("GNOME Target requires a usable %s command: %v", a.configuration.CommandName, safeErr)}, safeErr + return target.Availability{Evidence: fmt.Sprintf("GNOME Target requires a usable %s command: %v", a.configuration.CommandName, safeErr)}, settings{}, safeErr } present := make(map[string]bool) for _, schema := range strings.Fields(output) { @@ -364,49 +367,60 @@ func (a *Adapter) detectAvailability(ctx context.Context) (target.Availability, } for _, schema := range requiredSchemas { if !present[schema] { - return target.Availability{Evidence: "required GNOME settings schema is unavailable: " + schema}, nil + return target.Availability{Evidence: "required GNOME settings schema is unavailable: " + schema}, settings{}, nil } } - if _, err := a.readSettings(ctx); err != nil { + current, err := a.readSettings(ctx) + if err != nil { safeErr := redact(err) - return target.Availability{Evidence: fmt.Sprintf("GNOME proxy schema is present but unusable for the desktop session: %v", safeErr)}, safeErr + return target.Availability{Evidence: fmt.Sprintf("GNOME proxy schema is present but unusable for the desktop session: %v", safeErr)}, settings{}, safeErr } - return target.Availability{Available: true, Evidence: "gsettings command and GNOME proxy schemas are usable for the owning user"}, nil + return target.Availability{Available: true, Evidence: "gsettings command and GNOME proxy schemas are usable for the owning user"}, current, nil } func (a *Adapter) Detect(ctx context.Context, active *profile.Profile) target.Detection { - availability, err := a.detectAvailability(ctx) - return target.Detection{Availability: availability, Inspection: a.Inspect(ctx, active), Err: err} + availability, current, availabilityErr := a.detectAvailability(ctx) + if !availability.Available && availabilityErr == nil { + return target.Detection{Availability: availability} + } + inspection, inspectionErr := a.inspectSettings(active, current, availabilityErr) + return target.Detection{Availability: availability, Inspection: inspection, Err: errors.Join(availabilityErr, inspectionErr)} } func (a *Adapter) Inspect(ctx context.Context, active *profile.Profile) target.Inspection { - metadata, managed, metadataErr := a.readRecovery() current, settingsErr := a.readSettings(ctx) + inspection, _ := a.inspectSettings(active, current, settingsErr) + return inspection +} + +func (a *Adapter) inspectSettings(active *profile.Profile, current settings, settingsErr error) (target.Inspection, error) { + metadata, managed, metadataErr := a.readRecovery() if managed || (metadataErr != nil && !errors.Is(metadataErr, fs.ErrNotExist)) { if metadataErr != nil { - return target.Inspection{State: target.Drifted, Evidence: "managed GNOME recovery metadata cannot be read"} + return target.Inspection{State: target.Drifted, Evidence: "managed GNOME recovery metadata cannot be read"}, redact(metadataErr) } if settingsErr != nil { - return target.Inspection{State: target.Drifted, Evidence: "managed GNOME settings cannot be read: " + redact(settingsErr).Error()} + return target.Inspection{State: target.Drifted, Evidence: "managed GNOME settings cannot be read: " + redact(settingsErr).Error()}, redact(settingsErr) } if fingerprint(current) == metadata.LastFingerprint && active != nil { desired, err := translate(*active) if err == nil && fingerprint(desired) == metadata.LastFingerprint { - return target.Inspection{State: target.Enabled, Evidence: "managed GNOME settings match the Active Profile"} + return target.Inspection{State: target.Enabled, Evidence: "managed GNOME settings match the Active Profile"}, nil } } - return target.Inspection{State: target.Drifted, Evidence: "managed GNOME settings do not match the Active Profile"} + return target.Inspection{State: target.Drifted, Evidence: "managed GNOME settings do not match the Active Profile"}, nil } if settingsErr != nil { - return target.Inspection{State: target.Disabled, Evidence: "GNOME settings could not be inspected: " + redact(settingsErr).Error()} + safeErr := redact(settingsErr) + return target.Inspection{State: target.Disabled, Evidence: "GNOME settings could not be inspected: " + safeErr.Error()}, safeErr } switch current.Mode { case "manual": - return target.Inspection{State: target.External, Evidence: "recognized external GNOME manual proxy mode is configured"} + return target.Inspection{State: target.External, Evidence: "recognized external GNOME manual proxy mode is configured"}, nil case "auto": - return target.Inspection{State: target.External, Evidence: "recognized external GNOME automatic proxy mode is configured"} + return target.Inspection{State: target.External, Evidence: "recognized external GNOME automatic proxy mode is configured"}, nil default: - return target.Inspection{State: target.Disabled, Evidence: "GNOME proxy mode is none; no recognized desktop proxy configuration detected"} + return target.Inspection{State: target.Disabled, Evidence: "GNOME proxy mode is none; no recognized desktop proxy configuration detected"}, nil } } diff --git a/internal/gnome/gnome_test.go b/internal/gnome/gnome_test.go index 9d63fc6..e706c03 100644 --- a/internal/gnome/gnome_test.go +++ b/internal/gnome/gnome_test.go @@ -108,6 +108,25 @@ func active() *profile.Profile { return &profile.Profile{Name: "office", Mode: profile.AdvancedMode, HTTPURL: "http://proxy.example:8080", HTTPSURL: "http://secure.example:8443", AllURL: "socks5://socks.example:1080", Bypass: []string{"localhost", ".example.com", "10.0.0.0/8", "2001:db8::1"}} } +func TestDetectClassifiesMalformedRecoveryMetadataAsFailureWithoutRepeatingSettingsProbe(t *testing.T) { + runner := newRunner() + a := adapter(runner, &memoryState{data: []byte("{malformed")}) + + detection := a.Detect(context.Background(), active()) + if detection.Err == nil || detection.Inspection.State != target.Drifted { + t.Fatalf("detection = %#v, want failed Drifted inspection", detection) + } + gets := 0 + for _, call := range runner.calls { + if len(call.Args) > 0 && call.Args[0] == "get" { + gets++ + } + } + if gets != 12 { + t.Fatalf("Detect issued %d gsettings get probes, want one 12-setting inspection", gets) + } +} + func TestAuthenticationPasswordSetFailureSuppressesRunnerErrorAndProfileArgument(t *testing.T) { const profilePassword = "GNOME-PROFILE-PASSWORD-07" r, state := newRunner(), &memoryState{} diff --git a/internal/shell/shell.go b/internal/shell/shell.go index a51a3ea..88ed7cd 100644 --- a/internal/shell/shell.go +++ b/internal/shell/shell.go @@ -207,33 +207,45 @@ func (a *Adapter) DiagnosticChecks(_ context.Context, _ *profile.Profile) []targ } func (a *Adapter) Detect(ctx context.Context, active *profile.Profile) target.Detection { - return target.Detection{Availability: a.Availability(ctx), Inspection: a.Inspect(ctx, active)} + inspection, err := a.inspect(active) + return target.Detection{Availability: a.Availability(ctx), Inspection: inspection, Err: redactError(err, active)} } +// Inspect retains the maintenance contract: it always returns the safest +// state supported by the evidence. Detect adds the suppressed read error so an +// interactive check cannot present unreadable state as a successful result. func (a *Adapter) Inspect(_ context.Context, active *profile.Profile) target.Inspection { + inspection, _ := a.inspect(active) + return inspection +} + +func (a *Adapter) inspect(active *profile.Profile) (target.Inspection, error) { metadata, managed, metadataErr := a.readRecovery() generated, generatedErr := a.files.ReadFile(a.configuration.EnvironmentFile) if managed || (metadataErr != nil && !errors.Is(metadataErr, fs.ErrNotExist)) { if metadataErr != nil { - return target.Inspection{State: target.Drifted, Evidence: "managed Shell recovery metadata cannot be read"} + return target.Inspection{State: target.Drifted, Evidence: "managed Shell recovery metadata cannot be read"}, fmt.Errorf("inspect Shell recovery metadata: %w", metadataErr) } if generatedErr != nil { - return target.Inspection{State: target.Drifted, Evidence: "managed Shell environment is missing or unreadable"} + return target.Inspection{State: target.Drifted, Evidence: "managed Shell environment is missing or unreadable"}, fmt.Errorf("inspect managed Shell environment: %w", generatedErr) } if active != nil && fingerprint(generated) == metadata.Fingerprint && fingerprint(renderEnabled(*active)) == metadata.Fingerprint { - return target.Inspection{State: target.Enabled, Evidence: "managed Shell environment matches the Active Profile"} + return target.Inspection{State: target.Enabled, Evidence: "managed Shell environment matches the Active Profile"}, nil } - return target.Inspection{State: target.Drifted, Evidence: "managed Shell environment does not match the Active Profile"} + return target.Inspection{State: target.Drifted, Evidence: "managed Shell environment does not match the Active Profile"}, nil + } + if generatedErr != nil && !errors.Is(generatedErr, fs.ErrNotExist) { + return target.Inspection{State: target.Disabled, Evidence: "Shell environment could not be inspected"}, fmt.Errorf("inspect Shell environment: %w", generatedErr) } if generatedErr == nil && recognizesProxy(generated) { - return target.Inspection{State: target.External, Evidence: "recognized proxy assignments exist in the Shell environment file"} + return target.Inspection{State: target.External, Evidence: "recognized proxy assignments exist in the Shell environment file"}, nil } for _, name := range proxyVariables { if value, ok := a.environment.LookupEnv(name); ok && value != "" { - return target.Inspection{State: target.External, Evidence: "recognized proxy variable " + name + " exists"} + return target.Inspection{State: target.External, Evidence: "recognized proxy variable " + name + " exists"}, nil } } - return target.Inspection{State: target.Disabled, Evidence: "no recognized Shell proxy configuration detected"} + return target.Inspection{State: target.Disabled, Evidence: "no recognized Shell proxy configuration detected"}, nil } func (a *Adapter) SetDesired(ctx context.Context, enabled bool, active *profile.Profile) target.OperationResult { diff --git a/internal/shell/shell_test.go b/internal/shell/shell_test.go index 1b122d0..b44be98 100644 --- a/internal/shell/shell_test.go +++ b/internal/shell/shell_test.go @@ -80,6 +80,24 @@ func TestShellDiagnosticsRejectSymlinkedPathsWithoutTouchingVictim(t *testing.T) } } +func TestDetectClassifiesMalformedRecoveryMetadataAsFailure(t *testing.T) { + a := newAdapter(t, "/bin/bash", nil) + if err := os.MkdirAll(filepath.Dir(a.RecoveryPath()), 0o700); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(a.RecoveryPath(), []byte("{malformed"), 0o600); err != nil { + t.Fatal(err) + } + + detection := a.Detect(context.Background(), activeProfile()) + if detection.Err == nil || detection.Inspection.State != target.Drifted { + t.Fatalf("detection = %#v, want failed Drifted inspection", detection) + } + if got := a.Inspect(context.Background(), activeProfile()); got.State != target.Drifted { + t.Fatalf("maintenance inspection = %#v, want established Drifted semantics", got) + } +} + func TestShellAvailabilitySupportsOnlyBashAndZsh(t *testing.T) { for _, tc := range []struct { shell string diff --git a/internal/tui/dashboard.go b/internal/tui/dashboard.go index 91eccda..f88942b 100644 --- a/internal/tui/dashboard.go +++ b/internal/tui/dashboard.go @@ -20,10 +20,14 @@ type targetCatalog struct { items []app.TargetSnapshot } -func newTargetCatalog(snapshot app.Snapshot, includeUnavailable bool) targetCatalog { +func newTargetCatalog(snapshot app.Snapshot, includeUnavailable bool, selectedID string) targetCatalog { items := make([]app.TargetSnapshot, 0, len(snapshot.Targets)) for _, item := range snapshot.Targets { - if item.Available || includeUnavailable || item.DetectionPhase != app.TargetReady { + // Available Targets are the routine default. If progressive detection + // proves the selected identity unavailable, keep that one row visible + // until the user selects another Target rather than silently moving the + // selection to a different product. + if item.Available || includeUnavailable || item.ID == selectedID || item.DetectionPhase != app.TargetReady { items = append(items, item) } } diff --git a/internal/tui/model.go b/internal/tui/model.go index cb6dd92..a92312c 100644 --- a/internal/tui/model.go +++ b/internal/tui/model.go @@ -68,6 +68,7 @@ type refreshDone struct { type targetDetectionProgress struct { update app.TargetDetectionUpdate updates <-chan app.TargetDetectionUpdate + closed bool } type profileOperationProgress struct { @@ -90,7 +91,7 @@ func waitForTargetDetection(updates <-chan app.TargetDetectionUpdate) tea.Cmd { return func() tea.Msg { update, ok := <-updates if !ok { - return targetDetectionProgress{update: app.TargetDetectionUpdate{Complete: true}, updates: updates} + return targetDetectionProgress{updates: updates, closed: true} } return targetDetectionProgress{update: update, updates: updates} } @@ -120,7 +121,7 @@ func NewModel(application app.Module, configure ...ModelOption) Model { application: application, presentation: terminalPresentation{theme: newSemanticTheme(options.theme, options.output)}, snapshot: snapshot, - selectedTargetID: newTargetCatalog(snapshot, false).firstID(), + selectedTargetID: newTargetCatalog(snapshot, false, "").firstID(), width: 80, height: 24, refreshing: detectionUpdates != nil, @@ -173,9 +174,17 @@ func (m Model) Update(message tea.Msg) (tea.Model, tea.Cmd) { if !m.refreshing || progress.updates != m.detectionUpdates { return m, nil } - if progress.update.Result.TargetID != "" { - m.applyTargetDetection(progress.update.Result) + if progress.closed { + // A stale Application generation closes without claiming an + // authoritative completion snapshot. + m.refreshing = false + m.detectionUpdates = nil + return m, m.deferredQuit() } + formerCatalog := newTargetCatalog(m.snapshot, m.showUnavailable, m.selectedTargetID) + formerIndex := formerCatalog.index(m.selectedTargetID) + m.snapshot = progress.update.Snapshot + m.selectedTargetID = newTargetCatalog(m.snapshot, m.showUnavailable, m.selectedTargetID).retain(m.selectedTargetID, formerIndex) if progress.update.Complete { m.refreshing = false m.detectionUpdates = nil @@ -189,10 +198,10 @@ func (m Model) Update(message tea.Msg) (tea.Model, tea.Cmd) { return m, nil } m.refreshing = false - formerCatalog := newTargetCatalog(m.snapshot, m.showUnavailable) + formerCatalog := newTargetCatalog(m.snapshot, m.showUnavailable, m.selectedTargetID) formerIndex := formerCatalog.index(m.selectedTargetID) m.snapshot = completed.result.Snapshot - m.selectedTargetID = newTargetCatalog(m.snapshot, m.showUnavailable).retain(m.selectedTargetID, formerIndex) + m.selectedTargetID = newTargetCatalog(m.snapshot, m.showUnavailable, m.selectedTargetID).retain(m.selectedTargetID, formerIndex) if completed.result.Err != nil { m.status = "Error: " + completed.result.Err.Error() } else { @@ -356,7 +365,7 @@ func (m Model) updateMain(key tea.KeyMsg) (tea.Model, tea.Cmd) { } return m, nil } - catalog := newTargetCatalog(m.snapshot, m.showUnavailable) + catalog := newTargetCatalog(m.snapshot, m.showUnavailable, m.selectedTargetID) switch key.String() { case "p": if !m.busy() { @@ -394,7 +403,7 @@ func (m Model) updateMain(key tea.KeyMsg) (tea.Model, tea.Cmd) { if m.dashboardNavigationAvailable() { formerIndex := catalog.index(m.selectedTargetID) m.showUnavailable = !m.showUnavailable - m.selectedTargetID = newTargetCatalog(m.snapshot, m.showUnavailable).retain(m.selectedTargetID, formerIndex) + m.selectedTargetID = newTargetCatalog(m.snapshot, m.showUnavailable, m.selectedTargetID).retain(m.selectedTargetID, formerIndex) m.detailsOffset = 0 } case "up", "k": @@ -681,28 +690,10 @@ func (m Model) availableTargets() []app.TargetSnapshot { } func (m *Model) retainTargetSelection() { - catalog := newTargetCatalog(m.snapshot, m.showUnavailable) + catalog := newTargetCatalog(m.snapshot, m.showUnavailable, m.selectedTargetID) m.selectedTargetID = catalog.retain(m.selectedTargetID, catalog.index(m.selectedTargetID)) } -func (m *Model) applyTargetDetection(result app.TargetDetectionResult) { - formerCatalog := newTargetCatalog(m.snapshot, m.showUnavailable) - formerIndex := formerCatalog.index(m.selectedTargetID) - for i := range m.snapshot.Targets { - if m.snapshot.Targets[i].ID != result.TargetID { - continue - } - m.snapshot.Targets[i].DetectionPhase = result.Phase - m.snapshot.Targets[i].DetectionErr = result.Err - m.snapshot.Targets[i].Available = result.Availability.Available - m.snapshot.Targets[i].AvailabilityEvidence = result.Availability.Evidence - m.snapshot.Targets[i].State = result.Inspection.State - m.snapshot.Targets[i].Evidence = result.Inspection.Evidence - break - } - m.selectedTargetID = newTargetCatalog(m.snapshot, m.showUnavailable).retain(m.selectedTargetID, formerIndex) -} - func (f formState) labels() []string { if f.profile.Mode == profile.AdvancedMode { return []string{"Name", "HTTP URL", "HTTPS URL", "ALL/SOCKS URL", "Bypass"} diff --git a/internal/tui/pages.go b/internal/tui/pages.go index ec603b8..9e71d3c 100644 --- a/internal/tui/pages.go +++ b/internal/tui/pages.go @@ -20,7 +20,7 @@ func (p terminalPresentation) mainView(model presentationState, width, height in p.theme.title.Render("proxyctl"), activeProfile, p.stateCountSummary(model.snapshot)) } - catalog := newTargetCatalog(model.snapshot, model.showUnavailable) + catalog := newTargetCatalog(model.snapshot, model.showUnavailable, model.selectedTargetID) footer := p.dashboardFooter(model, catalog) if model.targetConfirm && !model.applying { footer = "y: confirm n/Esc: cancel" diff --git a/internal/tui/progressive_detection_test.go b/internal/tui/progressive_detection_test.go index a65aed4..cecd950 100644 --- a/internal/tui/progressive_detection_test.go +++ b/internal/tui/progressive_detection_test.go @@ -8,6 +8,7 @@ import ( tea "github.com/charmbracelet/bubbletea" "proxyctl/internal/app" + "proxyctl/internal/profile" "proxyctl/internal/target" ) @@ -17,7 +18,9 @@ type progressiveDetectionModule struct { begins int } -func (m *progressiveDetectionModule) Snapshot() app.Snapshot { return m.snapshot } +func (m *progressiveDetectionModule) Snapshot() app.Snapshot { + return cloneDetectionSnapshot(m.snapshot) +} func (m *progressiveDetectionModule) Dispatch(app.Intent) app.Result { return app.Result{Snapshot: m.snapshot} } @@ -27,7 +30,7 @@ func (m *progressiveDetectionModule) BeginTargetDetection() (app.Snapshot, <-cha m.snapshot.Targets[i].DetectionPhase = app.TargetChecking m.snapshot.Targets[i].DetectionErr = nil } - return m.snapshot, m.streams[m.begins-1] + return cloneDetectionSnapshot(m.snapshot), m.streams[m.begins-1] } func TestCompleteModelStreamsStartupAndRefreshByTargetWithoutMovingSelection(t *testing.T) { @@ -58,7 +61,7 @@ func TestCompleteModelStreamsStartupAndRefreshByTargetWithoutMovingSelection(t * t.Fatal("startup did not wait for progressive detection") } - startup <- detectionUpdate(module.snapshot, "docker", app.TargetReady, true, target.Enabled, "docker enabled", nil) + startup <- module.detectionUpdate("docker", app.TargetReady, true, target.Enabled, "docker enabled", nil) model, command = applyDetectionCommand(t, model, command) view := model.View() assertOrder(t, view, "Shell", "APT", "Docker") @@ -68,21 +71,18 @@ func TestCompleteModelStreamsStartupAndRefreshByTargetWithoutMovingSelection(t * } failure := errors.New("apt-config inspection failed") - startup <- detectionUpdate(module.snapshot, "shell", app.TargetDetectionFailed, false, "", "", failure) + startup <- module.detectionUpdate("shell", app.TargetDetectionFailed, false, "", "", failure) model, command = applyDetectionCommand(t, model, command) assertUsableFrame(t, model.View(), 120, 36, "! Check failed Shell", "> … Checking APT", "● Enabled Docker") - startup <- detectionUpdate(module.snapshot, "apt", app.TargetReady, true, target.External, "external APT proxy", nil) + startup <- module.detectionUpdate("apt", app.TargetReady, false, target.Disabled, "apt-config is unavailable", nil) model, command = applyDetectionCommand(t, model, command) - startup <- app.TargetDetectionUpdate{Complete: true} + startup <- app.TargetDetectionUpdate{Snapshot: cloneDetectionSnapshot(module.snapshot), Complete: true} model, command = applyDetectionCommand(t, model, command) if command != nil || model.selectedTargetID != "apt" { t.Fatalf("startup completion command=%v selection=%q", command != nil, model.selectedTargetID) } - assertUsableFrame(t, model.View(), 120, 36, "! Check failed Shell", "> ◇ External APT", "● Enabled Docker") - model = updateKey(model, "up") - assertUsableFrame(t, model.View(), 120, 36, "> ! Check failed Shell", "Check error: apt-config inspection failed") - model = updateKey(model, "down") + assertUsableFrame(t, model.View(), 120, 36, "! Check failed Shell", "> ○ Unavailable APT", "● Enabled Docker", "Evidence: apt-config is unavailable") updated, command := model.Update(keyMessage("r")) model = updated.(Model) @@ -91,7 +91,7 @@ func TestCompleteModelStreamsStartupAndRefreshByTargetWithoutMovingSelection(t * } assertUsableFrame(t, model.View(), 120, 36, "… Checking Shell", "> … Checking APT", "… Checking Docker") - refresh <- detectionUpdate(module.snapshot, "apt", app.TargetReady, true, target.Enabled, "apt enabled", nil) + refresh <- module.detectionUpdate("apt", app.TargetReady, true, target.Enabled, "apt enabled", nil) model, command = applyDetectionCommand(t, model, command) assertUsableFrame(t, model.View(), 120, 36, "… Checking Shell", "> ● Enabled APT", "… Checking Docker") if model.selectedTargetID != "apt" { @@ -99,12 +99,26 @@ func TestCompleteModelStreamsStartupAndRefreshByTargetWithoutMovingSelection(t * } } -func detectionUpdate(snapshot app.Snapshot, id string, phase app.TargetDetectionPhase, available bool, state target.State, evidence string, err error) app.TargetDetectionUpdate { - return app.TargetDetectionUpdate{Result: app.TargetDetectionResult{ - TargetID: id, Phase: phase, - Availability: target.Availability{Available: available, Evidence: evidence}, - Inspection: target.Inspection{State: state, Evidence: evidence}, Err: err, - }} +func (m *progressiveDetectionModule) detectionUpdate(id string, phase app.TargetDetectionPhase, available bool, state target.State, evidence string, err error) app.TargetDetectionUpdate { + for i := range m.snapshot.Targets { + if m.snapshot.Targets[i].ID != id { + continue + } + m.snapshot.Targets[i].DetectionPhase = phase + m.snapshot.Targets[i].DetectionErr = err + m.snapshot.Targets[i].Available = available + m.snapshot.Targets[i].AvailabilityEvidence = evidence + m.snapshot.Targets[i].State = state + m.snapshot.Targets[i].Evidence = evidence + break + } + return app.TargetDetectionUpdate{Snapshot: cloneDetectionSnapshot(m.snapshot), TargetID: id} +} + +func cloneDetectionSnapshot(snapshot app.Snapshot) app.Snapshot { + snapshot.Targets = append([]app.TargetSnapshot(nil), snapshot.Targets...) + snapshot.Profiles = append([]profile.Profile(nil), snapshot.Profiles...) + return snapshot } func applyDetectionCommand(t *testing.T, model Model, command tea.Cmd) (Model, tea.Cmd) {