Modularize runtime seams across CLI, engine, platform, and TUI#11
Merged
Modularize runtime seams across CLI, engine, platform, and TUI#11
Conversation
batu3384
changed the title
There was a problem hiding this comment.
Pull request overview
This PR refactors SIFT’s runtime seams across the CLI, engine, platform adapters, and TUI to reduce mixed responsibilities and hardens the local/CI “quality gate” by adding pinned lint tooling and more structured smoke assertions.
Changes:
- Introduces a pinned lint tool bootstrap (staticcheck + shellcheck) and wires
make lintintoquality-gateand CI. - Splits large TUI and CLI modules into focused runtime/viewmodel/bootstrap files and adds/updates tests for the new seams.
- Refactors engine execution orchestration, health snapshot collection, and native uninstall command parsing into dedicated units.
Reviewed changes
Copilot reviewed 59 out of 59 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Adds lint targets/tool bootstrap and runs lint as part of quality-gate. |
| internal/tui/validation_test.go | Strengthens protected-path validation assertions. |
| internal/tui/uninstall.go | Removes viewmodel helpers from the uninstall monolith. |
| internal/tui/uninstall_viewmodel.go | New uninstall viewmodel helpers extracted from uninstall view. |
| internal/tui/theme.go | Refactors style derivation in shared theme/render helpers. |
| internal/tui/plan_runtime.go | Adds runtime/update/view logic for status/progress/result models. |
| internal/tui/plan_flow_viewmodel.go | Moves plan-flow viewmodel logic out of render file. |
| internal/tui/plan_flow_render.go | Keeps plan-flow file focused on rendering (removes viewmodel helpers). |
| internal/tui/home_viewmodel.go | New home viewmodel helpers extracted from home rendering. |
| internal/tui/home_render.go | Removes viewmodel logic from home render file (now calls extracted helpers). |
| internal/tui/doctor.go | Minor runtime simplification using slice spread append. |
| internal/tui/app.go | Splits app model bootstrap/runtime/view routing into dedicated seams. |
| internal/tui/app_view.go | New route-based view dispatch helper for the app router. |
| internal/tui/app_test.go | Updates/extends tests to cover refactored runtime seams and bootstrap. |
| internal/tui/app_runtime.go | New shared runtime handlers extracted from the main Update loop. |
| internal/tui/app_plan_flow.go | Extracts plan-load handling and pending-state reset helpers. |
| internal/tui/app_help.go | Removes help viewmodel helpers from app help rendering file. |
| internal/tui/app_help_viewmodel.go | New help/footer bindings and helper logic extracted from app_help.go. |
| internal/tui/app_bootstrap.go | New centralized app model constructor and initial plan/result seeding. |
| internal/tui/analyze_browser.go | New analyze browser route model (interactive analyze flow). |
| internal/platform/current_windows_catalog.go | New Windows curated roots/protected paths/tasks/diagnostics catalog split. |
| internal/platform/current_windows_apps.go | New Windows app discovery/remnant discovery split. |
| internal/platform/current_darwin_apps.go | New macOS app discovery/remnant discovery split. |
| internal/platform/admin_session.go | Moves runtime primitives out of admin session orchestration. |
| internal/platform/admin_session_runtime.go | New runtime primitives for admin session/keepalive and GUI prompting. |
| internal/engine/native.go | Removes native command parsing helpers (moved to a dedicated file). |
| internal/engine/native_parse.go | New native uninstall command parsing and validation seam. |
| internal/engine/health.go | Removes Snapshot assembly from the large health file (moved to helper). |
| internal/engine/health_snapshot.go | New snapshot collector for assembling SystemSnapshot probes. |
| internal/engine/execution_runner.go | New execution orchestration runner emitting item/section progress. |
| internal/engine/execution_progress.go | New progress/section shaping helpers for execution status detail. |
| internal/engine/execution_helpers_test.go | Adds test coverage for ci-safe managed command blocking behavior. |
| internal/cli/root.go | Removes large plan-flow/interactive/config helpers (moved to new files). |
| internal/cli/root_plan_flow.go | New CLI plan-flow runner (plain/JSON/TUI routing + execution). |
| internal/cli/root_interactive.go | New extracted TUI callback wiring for interactive runs. |
| internal/cli/root_config.go | New extracted config persistence + whitelist/update-channel helpers. |
| internal/cli/output.go | Centralizes output-mode policy (plain vs JSON vs TUI) and --yes gating logic. |
| internal/cli/output_test.go | Adds tests for output-mode selection and --yes gating behavior. |
| install.sh | Refactors platform detection parsing to avoid positional set -- usage. |
| hack/windows_smoke.ps1 | Introduces structured assertion helpers; strengthens JSON-first smoke checks. |
| hack/macos_smoke.sh | Introduces JSON assertion helpers; reduces fragile text matching. |
| hack/install_dev_tools.sh | Adds pinned staticcheck/shellcheck bootstrap into .tmp/tools. |
| hack/capture_readme_screens.sh | Switches shebang to bash for broader compatibility. |
| docs/TESTING.md | Documents make lint and lint tool bootstrap behavior in the quality gate. |
| docs/ARCHITECTURE.md | Updates architecture documentation to reflect new seams and output policy. |
| .github/workflows/ci.yml | Adds macOS lint tool install + lint step to CI. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+28
to
+39
| ./hack/install_dev_tools.sh all | ||
|
|
||
| lint: lint-go lint-shell | ||
|
|
||
| lint-go: | ||
| chmod +x ./hack/install_dev_tools.sh | ||
| ./hack/install_dev_tools.sh staticcheck | ||
| PATH="$(TOOLS_BIN):$$PATH" staticcheck ./... | ||
|
|
||
| lint-shell: | ||
| chmod +x ./hack/install_dev_tools.sh | ||
| ./hack/install_dev_tools.sh shellcheck |
There was a problem hiding this comment.
The lint targets introduce TOOLS_DIR/TOOLS_BIN, but the install script keys off TOOL_ROOT (defaulting to .tmp/tools). If a developer overrides TOOLS_DIR (e.g., make TOOLS_DIR=/custom lint), the script will still install to the default directory while PATH is set to the overridden TOOLS_BIN, causing staticcheck/shellcheck not to be found. Consider exporting TOOL_ROOT=$(TOOLS_DIR) when invoking hack/install_dev_tools.sh (or have the script honor TOOLS_DIR) so the install location and PATH stay aligned.
Suggested change
| ./hack/install_dev_tools.sh all | |
| lint: lint-go lint-shell | |
| lint-go: | |
| chmod +x ./hack/install_dev_tools.sh | |
| ./hack/install_dev_tools.sh staticcheck | |
| PATH="$(TOOLS_BIN):$$PATH" staticcheck ./... | |
| lint-shell: | |
| chmod +x ./hack/install_dev_tools.sh | |
| ./hack/install_dev_tools.sh shellcheck | |
| TOOL_ROOT="$(TOOLS_DIR)" ./hack/install_dev_tools.sh all | |
| lint: lint-go lint-shell | |
| lint-go: | |
| chmod +x ./hack/install_dev_tools.sh | |
| TOOL_ROOT="$(TOOLS_DIR)" ./hack/install_dev_tools.sh staticcheck | |
| PATH="$(TOOLS_BIN):$$PATH" staticcheck ./... | |
| lint-shell: | |
| chmod +x ./hack/install_dev_tools.sh | |
| TOOL_ROOT="$(TOOLS_DIR)" ./hack/install_dev_tools.sh shellcheck |
Comment on lines
+80
to
+104
| switch { | ||
| case item.Status == domain.StatusProtected || item.Action == domain.ActionAdvisory: | ||
| r.appendResult(item, domain.OperationResult{ | ||
| FindingID: item.ID, | ||
| Path: coalesce(item.DisplayPath, item.Path), | ||
| Status: item.Status, | ||
| Reason: item.Policy.Reason, | ||
| Message: item.Recovery.Message, | ||
| }, ordinal) | ||
| case item.Action == domain.ActionNative: | ||
| nativeResult := r.service.executeNativeItem(r.ctx, r.plan, item, r.opts, func(phase domain.ProgressPhase, step, detail string) { | ||
| r.emitItemProgress(item, domain.OperationResult{}, phase, step, detail, ordinal) | ||
| }) | ||
| r.appendResult(item, nativeResult, ordinal) | ||
| if nativeResult.Status == domain.StatusCompleted { | ||
| r.result.Warnings = append(r.result.Warnings, nativeContinuationWarning(r.plan)) | ||
| } | ||
| case item.Action == domain.ActionCommand: | ||
| commandResult := r.service.executeManagedCommandItem(r.ctx, r.plan, item, func(phase domain.ProgressPhase, step, detail string) { | ||
| r.emitItemProgress(item, domain.OperationResult{}, phase, step, detail, ordinal) | ||
| }) | ||
| r.appendResult(item, commandResult, ordinal) | ||
| default: | ||
| r.executePathItem(item, ordinal) | ||
| } |
There was a problem hiding this comment.
executeItem doesn't account for items excluded by the review flow (e.g., ActionSkip / StatusSkipped). Those currently fall through to executePathItem and can be deleted even though the user explicitly excluded them. The runner should short-circuit excluded/skipped items and emit a StatusSkipped result (with an appropriate message) before any policy/fingerprint/apply logic runs.
Comment on lines
+55
to
+58
| if _, ok := seen[name]; ok { | ||
| continue | ||
| } | ||
| seen[name] = struct{}{} |
There was a problem hiding this comment.
seen is keyed by the raw app name casing (name := TrimSuffix(...); seen[name] = ...) but the resulting AppEntry.Name is lowercased. On a case-sensitive filesystem, this can emit duplicate entries that differ only by case (both will end up with the same lowercased Name). Consider normalizing the seen key (e.g., strings.ToLower(name)) to keep dedupe behavior consistent with the emitted Name (and with the Windows adapter which dedupes case-insensitively).
Suggested change
| if _, ok := seen[name]; ok { | |
| continue | |
| } | |
| seen[name] = struct{}{} | |
| normalizedName := strings.ToLower(name) | |
| if _, ok := seen[normalizedName]; ok { | |
| continue | |
| } | |
| seen[normalizedName] = struct{}{} |
Comment on lines
+98
to
+102
| for _, subEntry := range subEntries { | ||
| if subEntry.IsDir() && strings.HasSuffix(subEntry.Name(), ".app") { | ||
| scanDir(subDir, root.origin, root.requiresAdmin) | ||
| } | ||
| } |
There was a problem hiding this comment.
The nested scan logic calls scanDir(subDir, ...) once for each .app found inside subDir, but scanDir rereads and scans the entire directory each time. On folders with many apps this becomes O(n²) directory reads. Consider changing the inner loop to detect whether subDir contains any .app entries and then call scanDir at most once per subDir.
Suggested change
| for _, subEntry := range subEntries { | |
| if subEntry.IsDir() && strings.HasSuffix(subEntry.Name(), ".app") { | |
| scanDir(subDir, root.origin, root.requiresAdmin) | |
| } | |
| } | |
| hasAppBundle := false | |
| for _, subEntry := range subEntries { | |
| if subEntry.IsDir() && strings.HasSuffix(subEntry.Name(), ".app") { | |
| hasAppBundle = true | |
| break | |
| } | |
| } | |
| if hasAppBundle { | |
| scanDir(subDir, root.origin, root.requiresAdmin) | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This change restructures Sift's runtime architecture to reduce maintenance risk without intentionally changing the public CLI or TUI contract.
The work focuses on separating orchestration, view-model derivation, rendering, platform adapters, and quality-gate tooling into smaller seams so future changes can be made with lower regression risk.
Scope
CLI
The root command bootstrap was decomposed so command registration, interactive flow, and configuration helpers are no longer concentrated in a single large file.
Engine
Execution orchestration, progress shaping, health snapshot collection, and native command parsing were separated into dedicated runtime files. This makes the execution path easier to trace and easier to test in isolation.
Platform
The Darwin and Windows platform adapters were reduced to thin facades. App discovery, maintenance catalogs, and runtime-specific helpers were moved into focused files instead of being mixed into monolithic adapter implementations.
TUI
Large TUI modules were split into smaller route, runtime, and view-model seams. In particular, app shell, help/footer behavior, analyze, review, progress, result, status, home, and uninstall surfaces now separate state, derived display data, and rendering much more clearly.
Quality Gates
The validation pipeline was hardened with lint bootstrap tooling and more structured smoke coverage so local and CI verification are less fragile.
Why This Matters
Before this refactor, several core files had accumulated multiple responsibilities. That made changes harder to review, increased coupling across unrelated behaviors, and raised the likelihood of regressions when touching seemingly local code.
This refactor reduces that risk by making the codebase more modular, more testable, and more predictable to extend.
Validation
The final branch state was validated with:
go test ./...make lintmake smokeThese checks passed before merge.