diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 8989551..91c16b8 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -71,6 +71,7 @@ they disagree with an old comment, issue, or generated summary. source-code inference, and observed test results. A release-note keyword or passing help/flag contract test cannot establish full compatibility. Cite upstream advisories and name any untested interactive or TUI paths. -- If GitHub's automated Copilot review is enabled, apply these instructions to - its comments too. A human maintainer retains the decision to merge releases - and dependency updates. +- The `ProtectMain` ruleset requests Copilot review on new PRs. Request a fresh + review manually after later pushes when needed; automatic review of new + pushes and drafts is not enabled. A human maintainer retains the decision to + merge releases and dependency updates. diff --git a/.github/workflows/README.md b/.github/workflows/README.md index 5f42752..7db6a74 100644 --- a/.github/workflows/README.md +++ b/.github/workflows/README.md @@ -55,7 +55,11 @@ intermediate files. Linux AOT requires `clang` and `zlib1g-dev`. scanning where available. - GitHub Pages from Actions with custom domain `skillview.dev`. - Release immutability once all assets and checks are validated. -- A `main` ruleset requiring PR review and passing CI checks; allow the - maintainer to merge reviewed dependency PRs, not bots to bypass checks. +- `ProtectMain` targets the default branch and requests Copilot review on new + pull requests. As currently configured, it does not request another review + on new pushes or drafts, and it does not require a pull request or passing + checks before updating `main`. Enable those rules in repository settings if + they should become merge requirements; workflow files alone cannot enforce + them. `agent_docs/release-engineering.md` contains release development notes. diff --git a/AGENTS.md b/AGENTS.md index ceb734d..c8f8c52 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -148,23 +148,13 @@ the terminal, with both a full-screen TUI and scriptable CLI commands. replaced. `tests/SkillView.Tests/Build/PackageReferenceTests.cs` and `CliDispatcherHelpTests.VersionFlag_IncludesTerminalGuiVersion` hardcode the exact Terminal.Gui version string — update both alongside any bump. -- xunit was bumped 3.x → 4.0.0 (2026-08); the project didn't use any of the - now-obsolete parallelization APIs (`ParallelizeTestCollections`, - `[CollectionBehavior]`) so no test-code migration was needed. That bump also - triggered a required `global.json` change: `.NET 10 SDK` removed the legacy - VSTest-bridge `dotnet test` path entirely, so `global.json` now sets - `"test": {"runner": "Microsoft.Testing.Platform"}` to opt into the new MTP - `dotnet test` mode. See `agent_docs/running-tests.md` for the resulting - command-syntax changes: `--project` is now required (bare/positional project - paths no longer work), and filtered runs need xunit's MTP-native - `--filter-namespace`/`--filter-class`/`--filter-trait` flags passed straight - to `dotnet test`. For one exact method, use xunit's query filter, for example - `--filter-query "/*/Namespace/Class/Method"`; with the current xunit 4.0.0 - integration, `--filter-method` can silently select zero tests even when the - discovered fully qualified name matches. xunit's older - single-dash console-runner flags (`-namespace`, `-trait`, ...) and the - platform's own `--treenode-filter` are silently rejected by `dotnet test`'s - MTP handshake here and report "Zero tests ran" instead of erroring loudly. +- `global.json` selects `Microsoft.Testing.Platform` for xunit 4.0.1. Pass + `--project` for a specific project and use xunit's MTP-native + `--filter-namespace`/`--filter-class`/`--filter-trait` options for filtered + runs. For an exact method, use `--filter-query + "/*/Namespace/Class/Method"`. `--filter-method`, older single-dash xunit + flags, and `--treenode-filter` can silently select zero tests here. See + `agent_docs/running-tests.md` for examples. - `src/SkillView.Core/SkillView.Core.csproj` owns the default `TerminalGuiVersion` property. Keep the `PackageReference` on `Version="$(TerminalGuiVersion)"` so CI can override it via MSBuild without @@ -458,25 +448,18 @@ the terminal, with both a full-screen TUI and scriptable CLI commands. release workflow only generates Homebrew / WinGet artifacts when the repo variables (`HOMEBREW_TAP_ENABLED`, `HOMEBREW_TAP_REPO`, `WINGET_ENABLED`) are explicitly enabled. It does not push to a tap repo or submit to WinGet yet. -- Terminal.Gui `2.5.0` remains compatible with the modern - `Application.Create().Init()` lifecycle; the local - `UnconditionalSuppressMessage` workaround and temporary App-level warning mask - stay removed after a verification publish proved the App entrypoint no longer - needs them. Its planned API breaks remove legacy `ConfigurationManager`, make - `View.Text` nonvirtual, and move `IAcceptTarget` to `Terminal.Gui.Input`; keep - using `TuiConfigurationBuilder` and check these boundaries on later upgrades. +- Terminal.Gui `2.5.0` supports `Application.Create().Init()`. Keep using + `TuiConfigurationBuilder` and check the documented lifecycle, `View.Text`, + and `IAcceptTarget` API changes before a later package upgrade. - CI's standalone AOT smoke publish now promotes `IL2026`, `IL3050`, and `IL3053` to errors for `SkillView.App`; keep the gh extension's project-level suppression local until that host gets its separate re-evaluation. - Prefer `KeyBindings` for view-local command remaps like table preview shortcuts. Keep the current window/table `KeyDown` routing where the app is intentionally centralizing whole-screen actions (search/install/open/logs, - installed-screen filter/sort/remove, cleanup actions, etc.), not because of - the old `TableView` type-to-search swallowing bug. + installed-screen filter/sort/remove, cleanup actions, etc.). - On Terminal.Gui `2.5.0`, `TableView.CollectionNavigator = null` is the - supported way to disable type-to-search. Treat `#5232` as the fix for the old - printable-key swallowing behavior and prefer this documented path over the old - custom matcher workaround. + supported way to disable type-to-search. Do not restore the custom matcher. - Sanitize untrusted text before assigning it to preview/detail/log panes. `TerminalEscapeSanitizer` is now the shared UI-layer guard for remote preview markdown, search metadata, installed-skill detail markdown, cleanup/remove diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 638ea20..6aa5742 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -6,18 +6,17 @@ Thanks for helping improve SkillView. Report bugs and feature requests in [GitHu Build requirements: -- .NET SDK `10.0.100` or newer in the same feature band +- the .NET SDK selected by [`global.json`](global.json) - on Linux AOT publish: `clang` and `zlib1g-dev` ### Architecture -SkillView is intentionally small and explicit: +SkillView uses explicit composition and shared services: - **3 production projects**: `SkillView.Core`, `SkillView.App`, `SkillView.GhExtension` - **2 test projects**: `SkillView.Tests`, `SkillView.IntegrationTests` - shared logic lives in `SkillView.Core` - both executables call the same entry point -- no DI container - Native AOT-safe code paths by default Execution flow: @@ -31,9 +30,16 @@ Program.cs -> TUI: SkillViewApp.RunAsync(...) ``` -The TUI is a single Terminal.Gui `Window` that hosts a persistent shell: `TabBarView`, `ContextBarView`, and `StatusStripView`. The primary workflows live in embedded `DiscoverTabView`, `InstalledTabView`, and `ChangesTabView` instances under `src/SkillView.Core/Ui/Tabs/`; `SkillViewApp` coordinates tab activation and shared shell state while those views own their workspace layouts. Temporary drill-in workspaces such as `UpdatesTabView` and `DoctorTabView` are still embedded views, but they are not part of the persistent three-tab shell. Tab activation flips `Visible` flags; no nested `Application.Run` subloops are used for the primary workflows. Escalation paths (advanced install wizard, remove wizard, cleanup) keep their modal `Application.Run` semantics intentionally. +The TUI has a persistent Discover, Installed, and Changes shell. `SkillViewApp` +owns that shell and its application lifetime; `SkillViewWorkflowCoordinator` +orchestrates install, update, inventory, remove, cleanup, and Doctor work. +Views under `src/SkillView.Core/Ui/Tabs/` render the primary and temporary +workspaces. Advanced install, remove, and cleanup flows use owned modals. +See the [architecture guide](docs/architecture.md) for the current boundaries. -SkillView uses GitHub CLI's preview `gh skill` commands. It checks for `gh` 2.97.0 or newer and runs `gh skill --help` before enabling the app. GitHub CLI 2.99.0 added support for Pi's `PI_CODING_AGENT_DIR` and corrected Codex's user-scope install location. When GitHub CLI 2.101.0 or newer starts SkillView as an extension, SkillView uses the host-provided `GH_PATH` so subprocesses run through that same CLI executable. +SkillView requires GitHub CLI 2.97.0 or newer and checks `gh skill --help` +before enabling the app. When launched as an extension by GitHub CLI 2.101.0 +or newer, it uses the host-provided `GH_PATH` for subprocesses. ### Project layout @@ -52,18 +58,18 @@ SkillView uses GitHub CLI's preview `gh skill` commands. It checks for `gh` 2.97 ### Build and test ```bash -dotnet restore +dotnet restore --locked-mode dotnet build dotnet test --no-build ``` -The repo currently pins Terminal.Gui `2.5.0` (Terminal.Gui.Editor `2.5.7`) and -xUnit `4.0.1`. The release uses only stable package versions; the newer -Terminal.Gui development builds are intentionally not consumed. If you pulled -package changes, run `dotnet restore` before building so stale package assets -do not leave the test projects on xUnit 2.x. +NuGet lock files are committed for all projects. After intentionally changing +package versions, run `dotnet restore --force-evaluate`, commit the lock-file +changes, and confirm `dotnet restore --locked-mode` still passes. See +[`agent_docs/running-tests.md`](agent_docs/running-tests.md) for filtered tests. -There is no separate lint step. Build warnings and code-style violations are treated as errors. +CI also verifies formatting with `dotnet format SkillView.sln --no-restore +--verify-no-changes`, lints GitHub Actions, and checks the static site. ### Run locally diff --git a/agent_docs/running-tests.md b/agent_docs/running-tests.md index 2c3093c..96522db 100644 --- a/agent_docs/running-tests.md +++ b/agent_docs/running-tests.md @@ -1,6 +1,6 @@ # Running tests -The solution runs on xunit v3/v4's `dotnet test` integration with the .NET 10 SDK's +The solution runs on xunit v4's `dotnet test` integration with the .NET 10 SDK's `Microsoft.Testing.Platform` runner (opted in via `global.json`'s `test.runner`). A bare project path (e.g. `dotnet test tests/Foo/Foo.csproj`) no longer works — pass `--project ` explicitly, or run `dotnet test` from the repo root to @@ -18,7 +18,7 @@ product failure. `--filter-not-trait`, or `--filter-query "query"` for the query-filter language. For a single exact method, use a four-segment query such as `--filter-query "/*/SkillView.Tests.Ui/SearchAgentMetadataLoaderTests/FilterAsync_TimeoutContainsCallbackFailure"`. -On the current .NET 10/xunit 4.0.0 integration, both simple and fully qualified +On the current .NET 10/xunit 4.0.1 integration, both simple and fully qualified `--filter-method` values have been observed to reach the runner but silently select zero tests; do not count such a run as verification. Do **not** use xunit's older single-dash console-runner @@ -40,7 +40,7 @@ flags instead. 2.97.0) and the latest GitHub CLI release. A manual version override applies only to the latest lane. A failure opens an assigned compatibility issue. Keep the minimum lane equal to the app's hard minimum. -- **Automation checks:** `node --test .github/scripts/critical-dependencies.test.js`, +- **Automation checks:** `node --test .github/scripts/critical-dependencies.test.js .github/scripts/critical-dependency-assessment.test.js`, `python site/build.py`, and `node --check site/app.js`. CI also runs `actionlint` and dependency review for PRs. - For PTY-driven TUI checks, prefer the built binary and follow `agent_docs/tui-pty-testing.md`. diff --git a/agent_docs/tui-pty-testing.md b/agent_docs/tui-pty-testing.md index 68bae6e..c9eb796 100644 --- a/agent_docs/tui-pty-testing.md +++ b/agent_docs/tui-pty-testing.md @@ -125,9 +125,8 @@ Screen text can mislead because: - result tables truncate repo names - old frames remain in the PTY buffer -For startup smoke detection, accept the current `Search:` label as well as the -older `Query:` label so a harmless UI copy change does not masquerade as an -event-loop failure. +For startup smoke detection, wait for the `Search:` field rather than relying +on a transient status message. ### 3. Modal screens need unique wait conditions diff --git a/docs/architecture.md b/docs/architecture.md new file mode 100644 index 0000000..b6942a8 --- /dev/null +++ b/docs/architecture.md @@ -0,0 +1,45 @@ +# Architecture + +SkillView ships two executables: the standalone `skillview` app and the +`gh skillview` extension. Both call `EntryPoint.RunAsync` in `SkillView.Core`, +so CLI behavior and the terminal interface share the same services. + +## Entry and command paths + +- `src/SkillView.App/` and `src/SkillView.GhExtension/` contain the entrypoints. +- `src/SkillView.Core/Bootstrapping/` parses global arguments and creates + application services. `GhBinaryLocator` resolves `gh` and requires version + 2.97.0 or newer. `EnvironmentProbe` checks `gh skill --help` once; SkillView + does not probe individual flags at runtime. +- `src/SkillView.Core/Cli/` dispatches commands and formats text or JSON output. + The small command surface uses explicit parsers so it stays Native AOT safe. +- `src/SkillView.Core/Gh/` invokes `gh` with argument arrays. Install and + listing calls put trusted flags before `--` and selectors after it. + +## Terminal interface + +`SkillViewApp` owns the Terminal.Gui application lifetime, the persistent +Discover/Installed/Changes shell, and shared pane state. +`SkillViewWorkflowCoordinator` orchestrates install, update, inventory, remove, +cleanup, and Doctor workflows. The views in `src/SkillView.Core/Ui/Tabs/` +render those workflows. Dialogs and background tasks retain explicit lifetime +and cancellation owners; UI updates run through the active application loop. + +## Inventory and removal + +The inventory combines local skill directories with `gh skill list` data. +`PathIdentity` supplies path equality and containment. Real removal uses the +platform-specific `SecureRemovalBackend` after validation has captured the +target's native identity and its allowed scan root. It never falls back to +recursive path deletion when the secure backend is unavailable. See +[`AGENTS.md`](../AGENTS.md) and +[`agent_docs/ui-lifecycle-and-resource-bounds.md`](../agent_docs/ui-lifecycle-and-resource-bounds.md) +for the detailed safety contracts. + +## Builds and releases + +The solution targets .NET 10 and Terminal.Gui 2.5.0. Dependency lock files are +committed; CI restores in locked mode, tests on Linux, macOS, and Windows, and +smoke-publishes four Native AOT targets. Release tags trigger the asset, +checksum, provenance, and publication workflow described in +[`agent_docs/release-engineering.md`](../agent_docs/release-engineering.md). diff --git a/docs/decisions.md b/docs/decisions.md deleted file mode 100644 index fbc86b5..0000000 --- a/docs/decisions.md +++ /dev/null @@ -1,116 +0,0 @@ -# SkillView Decisions Log - -This file records durable architectural and workflow decisions for SkillView. - -## Format - -- Decisions are append-only. -- Each decision gets a stable `D-XXX` identifier. -- If a decision changes later, add a new entry that supersedes the old one instead of rewriting history. - ---- - -## D-001: Shared core with two tiny entrypoints (Active) - -**Context:** SkillView ships as both a standalone binary (`skillview`) and a GitHub CLI extension (`gh skillview`). The command surface and behavior need to stay aligned across both delivery modes without duplicating business logic. - -**Decision:** Keep all domain logic, CLI dispatch, `gh` adapters, logging, inventory, and TUI implementation in `SkillView.Core`. Keep `SkillView.App` and `SkillView.GhExtension` as tiny entrypoint projects that delegate to `EntryPoint.RunAsync(args)`. - -**Status:** Active - -**Effect:** -- `src/SkillView.App/Program.cs` -- `src/SkillView.GhExtension/Program.cs` -- `src/SkillView.Core/Bootstrapping/EntryPoint.cs` - -## D-002: Hand-rolled argument parsing over a framework parser (Active) - -**Context:** SkillView needs a small argv surface, AOT-friendly behavior, and custom parsing rules such as global flags that can appear before or after the subcommand (`--debug`) while other globals must stay before the subcommand (`--scan-root`). - -**Decision:** Use a hand-rolled `ArgParser` and explicit subcommand parsers in `CliDispatcher` rather than adopting a general-purpose command-line parsing framework. - -**Status:** Active - -**Effect:** -- `src/SkillView.Core/Bootstrapping/ArgParser.cs` -- `src/SkillView.Core/Cli/CliDispatcher.cs` -- `tests/SkillView.Tests/Bootstrapping/ArgParserTests.cs` -- `tests/SkillView.Tests/Cli/*` - -## D-003: Native AOT-safe by default (Active) - -**Context:** SkillView ships self-contained Native AOT binaries. That packaging model rewards explicit code paths and penalizes reflection-heavy infrastructure, implicit runtime discovery, and loosely bounded dependencies. - -**Decision:** Prefer AOT-safe patterns by default: explicit composition, source-generated or hand-rolled parsing where needed, warnings-as-errors during build, and conservative runtime assumptions such as invariant globalization. - -**Status:** Active - -**Effect:** -- `Directory.Build.props` -- `src/SkillView.Core/Ui/TuiServices.cs` -- `src/SkillView.Core/Bootstrapping/ArgParser.cs` -- `src/SkillView.Core/Inventory/FrontMatterParser.cs` - -## D-004: Capability-gated `gh skill` integration (Active) - -**Context:** `gh skill` is still evolving, and supported subcommands/flags differ by installed `gh` version. Blindly emitting flags based on documentation or assumptions would break older client versions. - -**Decision:** Probe `gh skill --help`, scan for known flag tokens, and only emit flags that the installed `gh` build actually supports. - -**Status:** Active - -**Effect:** -- `src/SkillView.Core/Gh/GhSkillCapabilityProbe.cs` -- `src/SkillView.Core/Gh/CapabilityProbeParser.cs` -- `src/SkillView.Core/Gh/CapabilityProfile.cs` -- `src/SkillView.Core/Gh/*Service.cs` - -## D-005: TUI lifecycle is owned by the host shell (Active) - -**Context:** SkillView’s top-level TUI hosts background work, nested dialogs, and application-wide state such as startup probing and status auto-clear timers. That work needs one clear owner for init, cancellation, crash logging, and disposal. - -**Decision:** Let `SkillViewApp` own the Terminal.Gui application lifetime for the main shell: create the app via `Application.Create().Init()`, wire the top-level window, own the run-lifetime cancellation token source, and dispose app/window resources when the shell exits. - -**Status:** Active - -**Effect:** -- `src/SkillView.Core/Ui/SkillViewApp.cs` -- `src/SkillView.Core/Bootstrapping/EntryPoint.cs` - -## D-006: View-local command bindings plus targeted app-level key routing (Active) - -**Context:** Terminal.Gui v2 tables consume some printable keys before they bubble, while SkillView still needs single-letter shortcuts for actions like preview, install, log toggle, and global navigation. Warp terminals also often deliver Enter as `Ctrl+J`. - -**Decision:** Bind view-local actions through `KeyBindings` where possible, keep selected app-level `KeyDown` routing where bubbling is insufficient, and explicitly support Warp’s `Ctrl+J` path for preview/activation. - -**Status:** Active - -**Effect:** -- `src/SkillView.Core/Ui/TuiHelpers.cs` -- `src/SkillView.Core/Ui/SkillViewApp.cs` -- `src/SkillView.Core/Ui/InstalledScreen.cs` - -## D-007: Disable table type-to-search through the native null path (Active) - -**Context:** SkillView uses single-letter shortcuts heavily. Table type-to-search can swallow those keys before they reach shortcut handlers. Older Terminal.Gui prereleases required a custom matcher workaround, but current 2.0.2-develop builds restore the documented `CollectionNavigator = null` behavior. - -**Decision:** Disable `TableView` type-to-search by setting `CollectionNavigator = null` and treat the old custom matcher approach as superseded. - -**Status:** Active - -**Effect:** -- `src/SkillView.Core/Ui/TuiHelpers.cs` -- `tests/SkillView.Tests/Ui/TuiHelpersTests.cs` - -## D-008: Best-effort file logging with stderr streaming in CLI debug mode (Active) - -**Context:** SkillView needs persistent logs for diagnosing TUI problems, but logging failures must never prevent the app from starting. CLI users also need immediate visibility when debugging command runs. - -**Decision:** Attach a file log sink on a best-effort basis, fall back to memory-only logging if sink creation fails, and stream formatted log lines to stderr when `--debug` is used in CLI mode. - -**Status:** Active - -**Effect:** -- `src/SkillView.Core/Bootstrapping/EntryPoint.cs` -- `src/SkillView.Core/Logging/FileLogSink.cs` -- `src/SkillView.Core/Logging/Logger.cs` diff --git a/docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md b/docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md deleted file mode 100644 index 5cac5c7..0000000 --- a/docs/reviews/adversarial-concurrency-resource-cancellation-audit-2026-08-28.md +++ /dev/null @@ -1,2099 +0,0 @@ -# Adversarial concurrency, resource, cancellation, and cross-platform audit - -Date: 2026-08-28 -Branch reviewed: `feat/tui-usability-hardening` -Reviewed commit: `7d43336` (`fix: address concurrency review findings`) -PR follow-up reviewed: `df62a22` (`fix: close remaining lifecycle review gaps`), -merged by PR #11 as `40bbf0b` -Hardening branch: `fix/adversarial-hardening` -Second hardening branch: `fix/resource-lifecycle-hardening-2` -Third hardening branch: `fix/removal-lifecycle-hardening` -Fourth hardening branch: `fix/search-metadata-hardening` -Fifth hardening branch: `fix/final-adversarial-hardening` -Sixth hardening branch: `fix/native-removal-hardening` -Status: Portable remediation and the documented SkillView backlog are complete. -PRs #12 through #21 are merged; the post-PR #21 cancellation-owner lifecycle -follow-up is implemented on `fix/cancellation-source-lifecycle`. -The fifth batch completes install-modal ownership, root CLI cancellation and -deadlines, bounded post-kill waiting, volume-aware path identity, Esc focus -behavior, streaming CLI JSON, serialized static-UI tests, and bounded-resource -stress coverage. The sixth batch implements the separately designed native -removal boundary for Windows, macOS, and Linux and records the narrower -guarantee Unix can actually provide. - -## Executive summary - -The original repository-wide audit found 11 actionable issues: one critical, -four high, and six medium. The critical removal-boundary defect and the -shared-cache race were reproduced against the actual production classes rather -than inferred only from source inspection. - -The PR follow-up exposed two misses in the original audit's UI-lifecycle model: -entering Doctor bypassed tab-load cancellation, and leaving Updates did not -cancel the active update operation. Both were fixed in `df62a22`. A targeted -reassessment then found three additional deferred production issues and one -test-coverage weakness: Discover and Doctor work still lacks workspace-scoped -ownership, log initialization/replay has two snapshot races, Esc does not leave -all advertised filter inputs, and the LRU test proves only FIFO eviction. These -are documented after Finding 11. - -The critical and high findings should be treated as release blockers. The -medium findings should be addressed in the same hardening cycle because they -share lifecycle, cancellation, and bounded-resource concerns with the -release-blocking work. - -The original audit made no production changes. Remediation is now in progress -on `fix/adversarial-hardening`; completed work and current verification are -recorded below. - -## Scope and method - -The review covered: - -- All TUI shells, tabs, screens, dialogs, event handlers, and UI-dispatch paths. -- Application initialization, run-loop shutdown, cancellation, and disposal. -- Every asynchronous workflow and fire-and-forget call site. -- Cancellation source ownership and replacement/supersession helpers. -- `gh` process creation, output draining, cancellation, and termination. -- Inventory scanning, path normalization, symlink handling, cleanup, and removal. -- Logger retention, subscribers, disk sinks, rotation, and disposal. -- In-memory caches and shared mutable state. -- CLI cancellation and resource ownership. -- Windows, macOS, and Linux path, reparse-point, process, and file-sharing behavior. -- Existing unit/integration coverage and gaps around timing and high-volume inputs. - -The following verification was performed: - -- `dotnet build SkillView.sln --no-restore`: passed with zero warnings. -- The repository's configured analyzer/build gate passed with zero warnings. - A later verification with the broader, non-configured `AnalysisLevel=latest-all` - ruleset reported 190 warnings-as-errors across changed and unchanged code; - that ruleset needs a separate baseline and triage before it can be treated as - a meaningful regression gate. -- `dotnet test --no-build`: 556 of 556 tests passed. -- macOS ARM64 AOT publish: passed. -- Removal escape reproduction: confirmed with the real validator and removal service. -- Shared-cache stress reproduction: confirmed with 1,761 concurrent-operation exceptions. -- Repository working tree after the audit: clean. - -Foreign Windows and Linux runtime packs were not restored locally, so this -audit does not claim live Windows/Linux runtime execution. Cross-platform -findings come from shared production code, the repository's platform CI -configuration, documented .NET 10 behavior, and a live macOS reproduction where -noted. - -## Post-PR reassessment: why two lifecycle defects were missed - -The original audit correctly examined the cancellation-slot implementation and -shutdown lifetime, but it did not model the complete UI as a transition graph. -That distinction matters because a view can stop owning the screen without -being disposed: - -- `ActivateTab` performed tab cancellation, while `EnterDoctor` hid the same - tabs through a different route. -- Updates canceled work on disposal, but Esc only hid the persistent embedded - view; disposal never occurred. - -The audit also overclaimed that the Updates operation slot prevented stale -operation results. It prevented two update operations from starting at the -same time while the view stayed active, but it did not tie the operation to -view deactivation. That statement has been corrected in the "Existing -hardening" section. - -Future lifecycle reviews should require a transition matrix for every -workspace and modal. For each exit path, record: - -1. Which view becomes hidden, stopped, or disposed. -2. Which load, operation, timer, subscription, and queued UI callback it owns. -3. Which token is canceled and which task is awaited. -4. Which generation or ownership check protects a late completion. -5. Which global UI state the completion can change: focus, status, spinner, - preview, visibility, or enabled controls. - -Deterministic tests should exercise each transition while the operation is held -at three boundaries: before cancellation, after background completion but -before UI dispatch, and after UI dispatch is queued but before it runs. - -### PR #11 follow-up disposition - -- **File sink lock inversion:** confirmed and fixed. Subscription deactivation - now occurs outside the sink lock, with a deterministic deadlock regression - test. -- **Doctor bypassed tab cancellation:** confirmed and fixed through the shared - `CancelPendingTabWork` path. -- **Updates operation survived Esc/deactivation:** confirmed and fixed. Leaving - the view now cancels the update, restores controls, and rejects a canceled - operation's late success or error UI callback. - -The focused follow-up passed all 559 local tests plus Linux, macOS, and Windows -tests, AOT smoke publishing on all three platforms, and CodeQL. - -### Post-merge hardening checkpoint - -The first natural implementation checkpoint completes remediation-order items -1-6: - -1. Removal now uses an explicit, depth-bounded, cancellation-aware traversal - with one lazy enumerator frame per active depth rather than materialized - child arrays or pending sibling stacks. - Nested symbolic links, junctions, mount-point reparse points, broken links, - and cycles are deleted only as leaf links; containment and ancestor link - state are revalidated immediately before each destructive operation. This - closes the static packaged-link escape, but supported .NET path deletion is - not atomic against a hostile same-user process replacing an ancestor between - the last check and use; that stronger native design remains open. -2. `GhSkillListCache` serializes all state and shares one cancellable load per - key. Invalidation cancels in-flight loads, rejects stale completion, and the - final canceled waiter waits for subprocess cleanup. -3. `BackgroundTaskTracker` owns application fire-and-forget work. Shutdown - closes admission, cancels app/workspace lifetimes, drains admitted work, - and only then clears/disposes Terminal.Gui state. Direct UI dispatch remains - available before the first real run for unit helpers but is permanently - disabled after lifecycle entry. -4. Discover and Doctor now have activation lifetimes and generation checks. - Search and preview are canceled on Discover exit, Doctor probes are canceled - on Doctor exit, searches supersede earlier searches, opening logs cancels a - preview that could close them, and environment probes are single-flight. -5. `Logger.SubscribeWithReplay` establishes an atomic, sequence-numbered - replay/live boundary. Both the file sink and TUI use it, so concurrent - entries cannot fall into a snapshot gap or appear twice. -6. Logs now have per-message and total-retained-character budgets, compact - stderr excerpts, a character-bounded TUI queue, same-day size rotation, - numbered file parts, active-file exclusion from trimming, and delete-share - compatibility on Windows. - -Local verification at this checkpoint: - -- `dotnet build --no-restore`: passed with zero warnings. -- `dotnet test --no-build`: 613 of 613 tests passed, including the ANSI-driver - integration suite. -- New deterministic tests cover removal escapes/cycles/retargeting, - cancellation between entries in a 2,000-file directory, 100,000-operation - cache contention, invalidation cleanup, shutdown drain, workspace exits, - replay/live interleavings, message budgets, same-day file rotation, restart - with an oversized active file, active-file retention, logger sequence-gap - backpressure, search/preview commit invalidation, and parse-aware inventory - caching. -- PR #12's initial Linux, macOS, and Windows tests, AOT smoke publishes on all - three platforms, and CodeQL checks passed. Follow-up CI reruns after review - fixes are recorded on the PR. - -### PR #12 Copilot review disposition - -The first Copilot review generated four comments. Each was independently -reassessed: - -1. **Path check/use race — correct residual limitation, not representable as a - portable managed fix.** The static nested-link escape remains fixed, but - path revalidation is not atomic against hostile same-user mutation. The - threat-model boundary, .NET runtime evidence, and native follow-up are now - explicit in this report, `AGENTS.md`, and `RemoveService` documentation. -2. **Per-directory `ToArray()` — correct and fixed.** Traversal now advances a - lazy enumerator one entry at a time and checks cancellation between - `MoveNext` calls. -3. **Pending sibling stack — correct and fixed.** One enumerator frame is kept - per active depth, so retained traversal state is O(depth), not proportional - to all unvisited siblings across the active path. -4. **Cache invalidation releases waiters before loader cleanup — correct and - fixed.** Invalidation marks and cancels a flight without completing it. The - completion is published only after the loader returns from process - kill/drain cleanup; a deterministic held-cleanup test proves the waiter - remains incomplete until then. - -The second Copilot review surfaced one inline finding and two suppressed -findings from unchanged lines. All three were independently reassessed and -accepted: - -1. **Logger sequence-gap buffer — correct and fixed.** The per-observer - `SortedDictionary` could grow without relation to the bounded ring when the - thread owning sequence N paused before observer dispatch. Out-of-order - deliveries now apply synchronous monitor backpressure until the missing - sequence is delivered, so the logger retains no separate gap collection. - Recursive writes from an observer to the same logger are rejected before - assigning a sequence because they are incompatible with synchronous ordered - delivery. A deterministic test pauses sequence N before registration, - proves N+1 remains blocked, then verifies exact ordered delivery. -2. **Preview started from the old table during search — correct and fixed.** A - search already canceled the preview active at submission, but a user could - start another preview before the search completed. Successful result commit - now invalidates the preview gate again before replacing the table, preventing - that old-row preview from painting over the new result set. -3. **Malformed successful inventory output cached as empty — correct and - fixed.** Parsing now carries an explicit success signal into - `GhSkillListCache.LoadResult.ShouldCache`. Only a schema-valid payload, - including a genuine empty `[]`, is cacheable. Blank, malformed, non-array, - non-object-record, and missing-name payloads remain retryable. - -The third Copilot review generated one inline finding and two suppressed -findings. They describe two underlying ownership defects and were accepted: - -1. **Queued search/preview callbacks outlive request leases — correct and - fixed.** Terminal.Gui 2.4.17 does not install a UI synchronization context, - so continuations commonly reach `IApplication.Invoke` from a worker thread. - The old fire-and-forget dispatch returned immediately, allowing each `using` - request lease to dispose before the UI loop evaluated `IsCurrent`. Search - and preview success, error, timeout, and cleanup paths now use an awaitable, - cancellation-aware dispatch wrapper. The request method cannot exit and - dispose its lease until the queued callback runs or its owning workspace is - canceled. -2. **Preview cancellation can clear search-owned busy state — correct and - fixed.** The global boolean spinner had no owner. Search and preview now - register distinct monotonic busy operation IDs. Removing a preview owner - restores the newest remaining operation, including its status text, so - opening logs or completing a preview cannot clear a still-running search. - Workspace deactivation retains an explicit clear-all path. - -Deterministic coverage holds an awaitable dispatch callback outside the UI -loop and proves the request remains current until execution, then exercises the -real log-pane action while search and preview busy owners overlap and verifies -that search remains visible after preview cancellation. - -The fourth Copilot review generated three inline findings. All three were -independently reassessed and accepted: - -1. **Indirect logger recursion can deadlock A → B → A — correct and fixed.** - A single thread-static logger identity detected only direct recursion. A - callback on logger A could write to logger B, whose callback could then - write back to A and wait forever on A's in-flight sequence. Observer - delivery now maintains a reusable per-thread stack of every active logger, - rejecting any direct or indirect callback cycle before assigning another - sequence or mutating a ring. The stack avoids a per-entry hash-set - allocation, and a bounded regression test proves the two-logger cycle - returns without deadlock or an extra A entry. -2. **Canceled deferred Installed layout can be reported as `CRASH` — correct - and fixed.** The three post-population width-stabilization passes are - tracked background tasks. If app teardown canceled their queued UI dispatch, - the cancellation previously escaped into `BackgroundTaskTracker` and was - reported as an unhandled fault. The deferred operation now consumes - `OperationCanceledException` only when its own view/app lifetime token is - canceled; unrelated cancellation and faults still propagate. The other tab - dispatch paths were rechecked and already catch cancellation through their - linked load/operation lifetime tokens. -3. **Numeric `skillName` accepted and cached — correct and fixed.** The generic - optional-field reader intentionally tolerates numeric versions, but it must - not define record identity. The canonical or legacy name field must now be - a nonblank JSON string. Numeric, null, Boolean, and canonical-invalid plus - legacy-valid records are rejected as schema-incompatible and remain - retryable instead of entering the cache. - -The fifth Copilot review generated one inline finding and two suppressed -findings. All three were independently reassessed and accepted: - -1. **Cross-thread logger cycles bypass a thread-local logger stack — correct - and fixed.** With concurrent outer writes, thread 1 could hold logger A's - registration lock and enter B while thread 2 held B's registration lock and - entered A. Each thread's local stack contained only its own path, so neither - detected the opposing lock cycle. Logger observers are production sinks, - not producers, and none of SkillView's observers log. The contract now - rejects every logger write made while any observer callback is active on - that thread, before assigning a sequence or mutating a ring. A scalar - thread-local depth counter avoids per-entry allocation. A synchronized - two-producer regression reaches both callbacks concurrently and proves both - outer writes finish without nested entries or deadlock. -2. **Search continuation dereferences a disposed Discover lifetime — correct - and fixed.** Discover navigation exchanges, cancels, and disposes the - workspace source. Search now safely captures a usable token value before - its first await and uses only that struct for its request and every success, - timeout, and error dispatch. A source disposed before capture is rejected. -3. **Preview has the same disposed-source race — correct and fixed.** Preview - uses the same capture helper and stable token for request creation, - cancellation classification, and all UI dispatches. The repository-wide - source audit found no equivalent externally-disposed source dereference in - the tab cancellation-slot paths; their leases retain source ownership until - completion. Modal lifetime ownership remains separately tracked by Finding - 8 and is not conflated with this workspace-source race. - -### PR #12 final review and post-merge analogous reassessment - -The final Copilot review was labeled “needs a closer look” and contained one -suppressed finding. It was independently reassessed and accepted: - -1. **Aggregate log usage can exceed the budget while the active part grows — - correct and fixed on `fix/resource-lifecycle-hardening-2`.** The first append - after writer open/rotation established retention, but subsequent appends did - not revisit it until another rotation. Older parts could therefore remain - while active growth pushed aggregate usage over 50 MiB. `FileLogSink` now - keeps incremental retained-byte accounting and re-runs retention on the - first append that crosses the budget. If the active part alone is oversized - or an old part cannot be deleted, retry is deferred until bounded additional - growth so logging does not enumerate the directory on every line. A - regression test begins with old and active parts under budget, grows only the - active part, and proves the old part is removed before rotation. - -The earlier review misses were then used as bug-family prompts rather than -one-off patches. This found four report/code gaps: - -1. **Cancellation callbacks ran under ownership locks — correct and fixed.** - `CancellationOperationSlot` and `LatestRequestGate` called `Cancel()` while - holding their private gates. `Cancel()` synchronously invokes arbitrary - registered callbacks; a callback that waits for another thread to release - or query the lease creates the same lock/callback inversion class that the - logger reviews exposed. Ownership is now published under the lock, - cancellation runs outside it, and source disposal is deferred until an - in-progress cancellation returns. Leases capture the token struct at - construction. Deterministic tests have a cancellation callback wait for a - different thread to dispose the lease and prove it completes while the - callback is active. -2. **The disposed-source pattern remains present in install modals — already - tracked, report sharpened.** All three install surfaces use `async void` - handlers with a `using`-owned source and repeatedly access `lifetime.Token` - after awaits. If the synchronous dialog run returns first, the source is - disposed while the handler continues. This is the same failure family as the - Discover continuation bug, but it belongs to the already-scheduled modal - lifetime redesign in Finding 8 rather than this checkpoint. -3. **Cleanup classification had a second uncancellable lazy-directory walk — - correct and fixed with Finding 6.** The original inventory finding focused - on `LocalSkillScanner`; `CleanupClassifier` also enumerated scan-root - children without a token and placed only enumerable creation, not - `MoveNext`, inside its error boundary. Cleanup classification now checks its - owning token throughout and contains iteration-time I/O failures. -4. **The cache invoked its injected clock while holding the cache lock — - low production risk, fixed.** The production clock is `UtcNow`, but the - abstraction allowed an injected delegate to re-enter or coordinate with - cache operations under `_gate`. Time capture now occurs before lock entry; - loader-completion clock faults are converted into the shared load outcome - so waiters cannot be stranded. A deterministic callback test invalidates on - another thread while the clock is running and proves the cache lock is free. - -Second-branch local verification at this checkpoint: - -- `dotnet build --no-restore`: passed with zero warnings and zero errors. -- `dotnet test --no-build`: 625 of 625 tests passed, including the ANSI-driver - integration suite. -- macOS ARM64 AOT publish and `--version` smoke checks passed for both the - standalone app and gh extension. The first standalone restore emitted a - local NuGet vulnerability-cache access warning, but native compilation and - execution completed; CI will repeat the clean three-platform matrix. - -## Finding 1: recursive removal can delete outside the selected skill - -Severity: **Critical** - -Implementation status: **Static nested-link escape completed on -`fix/adversarial-hardening`; native identity pinning and opened-directory -removal completed on `fix/native-removal-hardening`.** - -Locations: - -- `src/SkillView.Core/Inventory/RemoveService.cs`, recursive enumeration around lines 106-121. -- `src/SkillView.Core/Inventory/RemoveValidator.cs`, ancestor-link validation around lines 78-87. - -### Current behavior - -`RemoveService` recursively enumerates files and directories with: - -```csharp -new EnumerationOptions -{ - RecurseSubdirectories = true, - AttributesToSkip = 0, - IgnoreInaccessible = true, -} -``` - -Setting `AttributesToSkip` to zero means reparse points are included. Recursive -.NET searches include symbolic links, junctions, and mounted-drive reparse -points. `RemoveService` then deletes every file returned by that traversal. - -`RemoveValidator` checks whether an ancestor between the known scan root and -the selected target is a link that escapes the root. It does not inspect links -inside the selected skill. A skill can therefore pass every validation rule -and still contain a child link to arbitrary external content. - -### Reproduction - -An isolated tree was created with: - -- A scan root containing a normal skill directory. -- A valid `SKILL.md` inside the skill. -- A nested directory symlink inside the skill pointing to an external directory. -- One file in the external directory. - -The real `RemoveValidator.Validate` returned: - -```text -allowed=True errors=0 -``` - -The real `RemoveService.Remove` dry run returned: - -```text -files=2 dirs=2 -``` - -There was only one physical file owned by the skill (`SKILL.md`). The second -file counted for deletion was the external file reached through the nested -link. A non-dry run would call `File.Delete` on that external file. - -### Impact - -- A user-confirmed removal can cross the displayed and validated target boundary. -- A malicious or accidentally packaged skill can cause unrelated user data to be deleted. -- A link to an ancestor can create excessive recursive traversal, path growth, - latency, and allocation before enumeration fails. -- Windows junctions and mounted paths are in scope in addition to POSIX links. - -### Post-implementation concurrent-mutation boundary - -PR #12's first Copilot review correctly identified that revalidating ancestors -immediately before `File.Delete` or `Directory.Delete` still leaves a -check-to-use instruction window. Another same-user process can replace an -ancestor with a link after the check and before the path-based operation. - -The static malicious-package defect reproduced above is fixed: links already -present in the skill are treated as leaves and never traversed. The remaining -race is a different threat model. .NET 10 exposes no supported portable API for -handle-relative enumeration and deletion with no-follow semantics. Its own -`Directory.Delete(..., recursive: true)` implementation is path-based on both -[Unix](https://github.com/dotnet/runtime/blob/v10.0.0/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Unix.cs#L478-L523) -and -[Windows](https://github.com/dotnet/runtime/blob/v10.0.0/src/libraries/System.Private.CoreLib/src/System/IO/FileSystem.Windows.cs#L307-L402), -so substituting that API would not close the window and would also lose -SkillView's cancellation and partial-failure behavior. - -The native batch now pins the selected target's volume/device and file/inode -identity during validation, applies containment and the remaining policy to -the captured canonical deletion address, and checks the identity again after -opening the target for execution. Windows enumerates each opened directory -handle, verifies every child's volume and full 128-bit file ID after opening it -with reparse-point semantics, and deletes the opened object with -`FileDispositionInfoEx`. Renaming or replacing an ancestor pathname therefore -cannot redirect enumeration or deletion. - -Unix canonicalizes the validated target, opens each path component and child -directory with `openat(..., O_NOFOLLOW | O_DIRECTORY)`, enumerates duplicated -opened directory descriptors, compares `st_dev`/`st_ino`, uses Linux -`openat2(RESOLVE_NO_XDEV)` to reject bind and filesystem mounts, refuses macOS -device changes, and deletes names relative to the held parent with `unlinkat`. -Replacing an ancestor or an observed directory cannot redirect recursive -traversal. Directory identity is checked again before removal. - -There is one narrower Unix limitation that must remain explicit. POSIX -`unlinkat` removes a name relative to a directory descriptor; it does not -provide a general "unlink this already-open inode" operation. A same-UID -attacker can theoretically replace any final entry name between the final -`fstatat` identity comparison and `unlinkat`, including a directory or the -selected root. The operation will not follow a replacement symlink, will not -recursively delete a replacement directory, and cannot escape through an -ancestor, but it can unlink an empty replacement at the final name. That -final-name interval is not fully atomic. Windows deletion is bound to the -opened object handle. This is a -platform capability boundary, not something another managed path check can -close. See the Linux man-pages documentation for -[`unlinkat`](https://man7.org/linux/man-pages/man2/unlinkat.2.html) and -Microsoft's documentation for -[`SetFileInformationByHandle`](https://learn.microsoft.com/windows/win32/api/fileapi/nf-fileapi-setfileinformationbyhandle). - -### Required remediation - -- Never recurse into entries with `FileAttributes.ReparsePoint`. -- Treat every nested link as a leaf and delete only the link itself. -- Revalidate canonical containment immediately before each destructive operation. -- Use an explicit bounded traversal so link handling and cancellation are visible. -- For a hostile same-user mutation threat model, design and audit native - handle-relative/no-follow deletion separately on Unix and Windows. -- Add regression tests for: - - External directory links. - - External file links. - - Broken links. - - Links to ancestors/cycles. - - Links whose targets change after validation. - - Windows directory junctions. - - Target replacement after validation. - - Child-directory replacement between observation and open. - -### Native-removal checkpoint - -`fix/native-removal-hardening` implements the platform boundary without shell -commands or recursive managed deletion: - -- `RemoveValidator` records the native identity of every allowed target; an - identity that cannot be captured is a hard refusal rather than a downgrade. -- Windows retains one 1 KiB enumeration buffer per active depth, opens entries - with reparse-point semantics, compares volume/full 128-bit file IDs, and - marks the opened handle for deletion. The stack and buffers remain O(depth). -- macOS and Linux retain one opened descriptor/`DIR*` per active depth and one - reusable 512-byte stat buffer for the whole operation. Device/inode checks - bind descent to the entry actually enumerated. Linux `openat2` also refuses - bind mounts; macOS refuses directory device changes. -- Cancellation is checked between entries and before descent/deletion; - existing throttled progress, exact partial counts, bounded errors, and batch - accounting remain unchanged. -- Deterministic tests replace the selected target after validation and replace - an observed child directory with an external link. Both original and - replacement data survive; traversal reports the identity change. - -### PR #17 Copilot follow-up - -The Balanced review found nine related gaps, all accepted and addressed in the -same native-removal batch: - -- Policy is now evaluated against the canonical address captured with the - target identity, including canonicalized scan-root comparisons. -- Windows uses the full 128-bit `FILE_ID_128` from `FileIdInfo` and - `FileIdExtdDirectoryInfo`, disposes an opened child handle if identity reads - fail, and treats `ERROR_NOT_SUPPORTED` as a reason to try the legacy - disposition API. -- Unix disposes an opened child descriptor if identity reads fail. The audit - and tests now correctly apply the final-name race to directories and the - selected root as well as non-directory leaves. -- Identity-pinned skill removals enter the secure backend before any current- - path symlink branch, so a replacement link is refused rather than removed. -- Both backends check cancellation at the last safe point before leaf and - directory deletion; Unix link removal does the same. Popped traversal frames - are disposed in `finally` blocks so cancellation at that boundary cannot - leak their handles. - -The subsequent review found one more correct gap in the cleanup-specific path: -the UI and CLI empty-directory validators constructed allowed validations -without an execution identity. A path replacement could therefore reach native -recursive removal with `expectedIdentity: null`. The fix centralizes empty- -directory validation in `RemoveValidator`, captures and canonical-policy-checks -its identity, refuses scan roots, and marks the operation with an empty-only -execution contract. Native traversal now refuses as soon as it observes a child -instead of deleting it, covering both replacement and same-directory population -after validation. `RemoveService` also refuses every real identity-less -directory operation. Broken-symlink cleanup is now explicitly marked link-only -and refuses if the entry changes kind before execution. Regression tests cover -identity-less synthetic validation, population after validation, and complete -directory replacement. - -The next Balanced review found three more valid object-binding and capability -gaps: - -1. **Windows canonical paths were lexical.** `Path.GetFullPath` retained - junction/symlink ancestors even though the file identity came from an opened - handle. Windows now obtains the fully resolved path from that handle with - `GetFinalPathNameByHandleW`, and scan-root canonicalization uses the same - opened-handle method before policy comparison. -2. **Link-only cleanup pinned neither the parent nor the link.** Broken-link and - agent-link validation now capture the canonical parent path, native parent - identity, final name, and native link identity. Execution reopens and verifies - the parent and link before deleting the descriptor-relative Unix name or the - exact Windows link handle. Replacement-link, replacement-parent, and - cancellation-boundary tests prove the wrong entry is preserved/refused. -3. **Linux discovered missing `openat2` support after mutation began.** Backend - selection now probes the exact `RESOLVE_BENEATH | RESOLVE_NO_SYMLINKS | - RESOLVE_NO_XDEV` request. Pre-5.6 kernels and seccomp denials disable secure - removal before validation, so nested traversal cannot discover the missing - capability only after root-level leaves were removed. - -A nearby analogue not called out by the review was fixed in the same pass: -Unix previously called `realpath` and then opened that string, retaining the -pre-open path even if a rename race changed which object was opened. Linux now -reads the final path from `/proc/self/fd`; macOS uses the fixed-signature -`fgetattrlist(ATTR_CMN_FULLPATH)` API with a bounded buffer and strict returned- -offset validation. Thus all three supported operating-system families bind the -policy address and identity to the same opened object. - -The first Linux CI run then exposed an additional identity subtlety: ext4 -immediately recycled the removed test symlink's inode, so device/inode alone -accepted the replacement even though the implementation had followed the -review literally. Unix link identity now also retains and compares `st_ctim` -seconds/nanoseconds. Directory traversal uses the same stronger comparison -between observations and at the destructive boundary; for the final directory -name it compares a fresh stat of the still-open descriptor to `fstatat`, since -deleting children legitimately changes the directory's change time. The -verified Linux x64 and ARM64 layouts both place those fields at bytes 104/112. - -The following review added three more valid findings, all consequences of the -same incomplete object-binding model: - -1. **Windows children were observed by handle but reopened by full path.** A - held directory could be renamed, its old path replaced by a junction, and a - matching hard link placed behind that junction. The full 128-bit file ID - would legitimately match while deletion removed the external hard-link name. - Every enumerated child and link is now opened relative to the held parent - with `NtOpenFile` and `OBJECT_ATTRIBUTES.RootDirectory`; display paths are no - longer reused as authority paths. -2. **Unix directory capture followed the final component.** `realpath(path)` - allowed an empty-directory candidate swapped to a symlink to be captured as - its destination. Destructive capture now resolves only the parent and opens - the final name with `O_NOFOLLOW | O_DIRECTORY`. Final-following behavior - remains only in the explicitly non-destructive canonicalization operation. -3. **Selected-directory identity omitted the new generation fields.** The prior - inode-reuse correction compared `st_ctim` for links and traversal - observations but intentionally preserved the old selected-directory behavior - to allow content changes after confirmation. That was the wrong safety - tradeoff. Selected directories and link parents now compare change time too; - any intervening namespace/content change causes refusal and revalidation. - -The durable review method is now explicit: map every native destructive path as -observe → reopen → compare → delete, identify the exact authority object at each -edge, and treat any pathname reconstructed after a parent handle is held as a -red flag. The corresponding adversarial matrix combines ancestor rename and -replacement with hard links, final-component symlink replacement, and immediate -identifier reuse instead of testing each mechanism in isolation. - -Windows CI then exposed the analogous generation mistake in the updated test -expectations: replacing a link could immediately reuse its file ID, and adding a -child changed an empty directory without changing its file ID. Treating that as -a Windows-only test exception would have preserved the same class of bug. The -Windows backend now captures `FILE_BASIC_INFO.CreationTime` and `ChangeTime` -alongside the full 128-bit ID for selected directories and validated parent/link -identities. Transient traversal entries deliberately use parent-relative open -plus full ID/type comparison: Windows directory-enumeration timestamps proved -unstable across ordinary traversal, and those entries were not independently -approved policy objects. The broader policy-boundary invariant is -platform-neutral: an -object ID identifies a filesystem slot, while the platform -change-time/generation signal binds the validated incarnation occupying that -slot. - -The final suppressed review body identified two additional valid policy-binding -gaps. First, `SKILL.md` and `.git` were checked through the captured canonical -pathname after the native identity handle had closed. An ancestor ABA swap could -therefore present a clean replacement for policy inspection and restore the -original, uninspected object for execution. Directory validation now returns a -single handle-bound snapshot containing identity, canonical address, skill-file -presence, `.git`-directory presence, and emptiness; the object generation is -checked before and after the relative inspection. Second, brokenness was checked -before link identity capture. Link validation now observes target existence -relative to the held parent and rechecks both the parent and named-link identity -around that observation, so replacing a broken link with a valid link refuses -validation. Deterministic Unix and Windows tests exercise both ABA sequences. - -The analogous review also revisited Windows access masks. Native opens requested -`FILE_WRITE_ATTRIBUTES` even though identity inspection requires read attributes -and handle disposition requires `DELETE`; the unrelated access bit could reject -otherwise-authorized removals and has been removed. - -A subsequent Linux review found that `/proc/self/fd` appends the unescaped text -` (deleted)` to an unlinked entry, even though the same suffix is legal in a -live filename. Final-path capture previously rejected every such suffix and -therefore refused valid skills or scan roots with that name. The Linux backend -now stats the returned absolute name without following its final component and -compares the full device, inode, type, and change-time generation with the open -descriptor before and after that lookup. A matching live name is accepted; -an absent, replaced, or unstable name fails closed. Regression tests cover both -the legitimate-name case and an unlinked directory colliding with a replacement -at the annotated pathname. - -The next review identified four further valid boundary and portability gaps. -First, target and scan-root canonicalization used separate opens, allowing a -scan-root symlink ABA to make both path comparisons authorize an external -target. Validation now opens and pins the matched scan root first, opens the -selected directory or link parent relative to that held object, derives both -canonical addresses from those handles, and verifies the root generation after -policy capture. Second, cleanup prevalidated an entire batch; unlinking the -first sibling changes their shared parent's native generation and invalidated -the remaining link identities. Cleanup now supplies a lazy validation sequence -that pins each candidate immediately before its removal. Third, Windows stripped -`\\?\` from volume-GUID paths and thereby made them relative; only drive-letter -paths now lose that prefix, while extended UNC conversion remains unchanged. -Fourth, non-destructive Unix canonicalization routed `/` through a destructive -parent helper that intentionally refuses filesystem roots. It now opens the -`realpath` result directly. Regression coverage exercises root retargeting on -Unix and Windows, sibling broken-link batches, volume-GUID normalization, and -filesystem-root canonicalization. - -The following Windows review found two valid defects in the legacy disposition -fallback. `FILE_DISPOSITION_INFO.DeleteFile` is a one-byte Win32 `BOOLEAN`, but -the managed structure used four-byte `UnmanagedType.Bool`, so filesystems that -require the fallback could reject the buffer size. The field is now a byte and -its native size is regression-tested. Also, the extended and legacy -`SetFileInformationByHandle` calls are two distinct destructive boundaries: -cancellation can arrive while the unsupported extended call returns. The -shared disposition state machine now checks cancellation immediately before -both calls, with deterministic tests proving an already-canceled request makes -no attempt and cancellation after the first failure suppresses the fallback. - -A broader pass found the same cancellation shape in the unsupported-platform -symlink fallback: `File.Delete` failure could flow directly into -`Directory.Delete`, and the synchronous caller converted cancellation into a -normal error report. Both path-based delete attempts now have their own token -check, and cancellation is published and rethrown. The remainder of the native -destructive path was re-inventoried: Windows has no other disposition fallback, -every `TryDeleteHandle` caller now supplies the operation token, and all three -Unix `unlinkat` sites already check the token immediately before the call. - -## Finding 2: `FileLogSink.Dispose` can deadlock - -Severity: **High** - -Status: **Fixed in `df62a22` / PR #11.** The replay/attach race described in -Finding 13 was subsequently completed on `fix/adversarial-hardening` / PR #12. - -Locations: - -- `src/SkillView.Core/Logging/FileLogSink.cs`, `Append` around lines 47-70. -- `src/SkillView.Core/Logging/FileLogSink.cs`, `Dispose` around lines 197-206. -- `src/SkillView.Core/Logging/Logger.cs`, observer invocation and deactivation around lines 107-135. - -### Current behavior - -`Logger.ObserverRegistration.Invoke` holds the registration lock while invoking -the sink callback. `FileLogSink.Append` then acquires the sink lock. - -`FileLogSink.Dispose` acquires the sink lock first and disposes the logger -subscription while still holding it. Subscription disposal waits for any -in-flight observer invocation by acquiring the registration lock. - -The resulting lock cycle is: - -1. Logging thread acquires the logger registration lock. -2. Logging thread enters `Append` and waits for the sink lock. -3. Disposal thread holds the sink lock. -4. Disposal thread calls subscription disposal and waits for the registration lock. - -Neither thread can proceed. - -### Secondary disposed-state race - -`Append` checks `_disposed` before entering the sink lock and does not recheck it -inside the lock. A direct call that observes `false`, pauses, and resumes after -disposal can call `EnsureWriter` and reopen the file after disposal completed. -The field is also neither volatile nor accessed exclusively under the lock. - -### Impact - -- Application shutdown can freeze indefinitely. -- CLI completion can hang in `EntryPoint` while disposing the file sink. -- A writer can be recreated after the sink is logically disposed. - -### Required remediation - -- Atomically detach/capture the subscription before acquiring the sink lock. -- Dispose the subscription without holding the sink lock. -- Mark the sink disposed under the sink lock. -- Recheck disposed state inside `Append` after acquiring the lock. -- Add deterministic tests that coordinate an in-flight append with disposal. -- Add a test proving no file is created or reopened after disposal returns. - -## Finding 3: late background callbacks can mutate disposed UI - -Severity: **High** - -Implementation status: **Completed on `fix/adversarial-hardening`; aggregate -active-growth enforcement completed on `fix/resource-lifecycle-hardening-2`.** - -Locations: - -- `src/SkillView.Core/Ui/SkillViewApp.cs`, `RunAsync` teardown around lines 214-227. -- `src/SkillView.Core/Ui/SkillViewApp.cs`, `Invoke` around lines 2114-2142. -- `src/SkillView.Core/Ui/SkillViewApp.cs`, `RunBackground` around lines 2253-2283. - -### Current behavior - -`RunBackground` starts discarded `Task.Run` tasks. The application does not -keep a registry of those tasks and does not await them during shutdown. - -`RunAsync` cancels the lifetime and then resets: - -```csharp -_hasRunLifetime = false; -_runLifetime = null; -_app = null; -``` - -It then disposes the window and application. `Invoke` executes its action -directly whenever `_app` is null and `_hasRunLifetime` is false. That fallback -is useful for pre-run unit helpers but becomes unsafe after a real run. - -Late startup probes, inventory scans, rescans, discovery work, search `finally` -blocks, and error handlers can therefore invoke UI actions directly from worker -threads after teardown began or completed. - -### Impact - -- Off-UI-thread Terminal.Gui mutation. -- Access to disposed controls. -- Timing-dependent `NotInitializedException`, `ObjectDisposedException`, or - native terminal-driver failures. -- Post-shutdown tasks can continue using services while `EntryPoint` disposes - the file logger. - -### Required remediation - -- Track every application-owned background task. -- Stop accepting new tasks once shutdown begins. -- Cancel the lifetime and await all tracked tasks before clearing `_app` or - disposing the window/application. -- Make the direct `Invoke` fallback valid only before the first real run, never - after the application has entered its lifecycle. -- Add tests that hold startup/search/rescan work across cancellation and verify - no callback touches UI after teardown. - -## Finding 4: the shared `gh skill list` cache is not thread-safe - -Severity: **High** - -Implementation status: **Completed on `fix/adversarial-hardening`.** - -Locations: - -- `src/SkillView.Core/Gh/GhSkillListCache.cs`, all operations around lines 10-45. -- `src/SkillView.Core/Gh/GhSkillListAdapter.cs`, shared adapter/cache ownership. -- `src/SkillView.Core/Ui/TuiServices.cs`, singleton service composition. - -### Current behavior - -The cache uses a normal `Dictionary` with no lock. -`TryGet`, `Store`, and `Invalidate` all mutate or read it directly. - -One adapter instance is shared across the application. Overlapping startup -inventory capture, tab loads, scope changes, update/install rescans, cleanup, -and explicit invalidation can access the dictionary concurrently. - -### Reproduction - -The actual internal cache was exercised with 250,000 mixed parallel `TryGet`, -`Store`, and `Invalidate` calls. It produced 1,761 exceptions. The first was: - -```text -System.ArgumentException: Destination array was not long enough. Check the -destination index, length, and the array's lower bounds. -``` - -### Impact - -- Normal overlapping TUI activity can crash the application. -- Cache state can be corrupted or entries can be lost. -- Simultaneous misses also spawn duplicate `gh skill list` processes. - -### Required remediation - -- Protect the complete cache state with one lock or use a concurrency-safe - cache implementation with equivalent atomic expiry behavior. -- Add per-key single-flight loading so concurrent misses share one process. -- Keep invalidation atomic with respect to reads and stores. -- Add high-contention tests for get/store/invalidate and simultaneous expiry. - -## Finding 5: log retention can consume multi-gigabyte memory and unbounded disk - -Severity: **High** - -Implementation status: **Completed on `fix/adversarial-hardening`.** - -Locations: - -- `src/SkillView.Core/Logging/Logger.cs`, entry retention around lines 36-55. -- `src/SkillView.Core/Subprocess/ProcessRunner.cs`, per-stream cap around lines 10 and 121-165. -- `src/SkillView.Core/Gh/GhSkillListAdapter.cs` and other `gh` adapters that log full stderr. -- `src/SkillView.Core/Logging/FileLogSink.cs`, trim scheduling around lines 104-158. -- `src/SkillView.Core/Ui/SkillViewApp.cs`, visible log queue around lines 1904-1953. - -### Memory behavior - -The logger limits only the number of entries. It does not limit message length -or total retained characters. `ProcessRunner` allows 1,048,576 characters per -captured stream, and multiple `gh` services interpolate the full stderr string -into warning entries. - -At the default 2,048 entries, repeated noisy failures can retain approximately -4 GiB of UTF-16 character storage before accounting for: - -- String and linked-list overhead. -- Regex redaction copies. -- Interpolated-string copies. -- Process-result strings. -- File formatting. -- The 512-entry visible log queue, which can retain roughly another 1 GiB of - formatted copies while the log pane is open. - -### Disk behavior - -The documented 50 MB budget is checked only when a writer is first opened or -rotated. A long-running same-day process can grow the active file without any -further budget check. - -If the application restarts on the same day with an already oversized current -file, trimming can select the active file: - -- Unix can unlink it while the writer continues writing to an invisible open file. -- Windows deletion fails because the file was opened with `FileShare.Read`, not - delete sharing. - -### Impact - -- Sustained failing commands can cause out-of-memory termination. -- The log pane magnifies retained memory. -- Logs can consume disk beyond the advertised budget. -- Unix and Windows behave differently when trimming the current file. - -### Required remediation - -- Apply a strict per-message character limit before inserting into the ring. -- Bound the ring by total retained characters/bytes in addition to entry count. -- Log concise stderr snippets rather than full captured streams. -- Bound visible-log text by both line count and total characters. -- Rotate files by size as well as date. -- Exclude the active file from deletion and rotate/close it before enforcing budget. -- Add same-day growth, restart, Windows sharing, and large-message tests. - -## Finding 6: inventory scanning blocks the UI and largely ignores cancellation - -Severity: **Medium** - -Implementation status: **Completed on -`fix/resource-lifecycle-hardening-2`.** - -Locations: - -- `src/SkillView.Core/Inventory/LocalInventoryService.cs`, `CaptureAsync` around lines 42-81. -- `src/SkillView.Core/Inventory/LocalSkillScanner.cs`, enumeration and reads around lines 48-124. -- `src/SkillView.Core/Inventory/SkillLockFileReader.cs`, full-file read around line 84. - -### Current behavior - -`LocalInventoryService.CaptureAsync` performs root resolution and the full -filesystem scan before reaching its first `await`. When an environment report -is already cached, tab activation reaches this scan synchronously on the UI -thread before the returned task can yield. - -The scanner does not accept a cancellation token. It reads every `SKILL.md` -completely, even though inventory needs only front matter. `SkillLockFileReader` -also reads complete files into byte arrays without a size limit. - -`Directory.EnumerateFileSystemEntries` is lazy. The code catches exceptions -around creation of the enumerable but performs `foreach` outside that catch. -ACL changes, directory removal, disconnection, or other errors raised during -iteration can escape and abort the scan. - -### Remediation checkpoint - -`LocalInventoryService` now schedules root resolution, filesystem scanning, -and package-lock enrichment on the thread pool and runs that work concurrently -with `gh skill list`. Cancellation is checked between roots, candidates, -bounded read chunks, lockfiles, and cleanup-classification entries. - -`LocalSkillScanner` reads at most 64 KiB from each `SKILL.md`, enough for normal -front matter without allocating the entire third-party document. -`SkillLockFileReader` rents a bounded buffer, reads at most 1 MiB plus one byte, -and rejects oversized manifests. Both use delete-sharing so concurrent package -updates/removal do not unnecessarily pin files on Windows. Lazy enumeration -now catches failures from the actual `MoveNext` call, not only enumerable -creation. Focused tests cover mid-scan cancellation, a closing front-matter -fence beyond the byte bound, oversized lockfiles, cancellation before lockfile -I/O, and a simulated iteration-time disconnect. - -### Impact - -- Frozen TUI during installed/changes/updates refreshes. -- Cancellation does not stop local I/O promptly. -- A huge third-party `SKILL.md` or lock file can cause large allocations. -- Network mounts, removable media, and Windows ACL behavior increase latency - and failure likelihood. - -### Required remediation - -- Execute filesystem scanning away from the UI thread. -- Pass a cancellation token through resolver/scanner/lock-reader operations. -- Check cancellation per root and candidate. -- Read a bounded front-matter prefix rather than the complete Markdown file. -- Establish and enforce a lock-file size limit. -- Catch errors around actual enumeration/move-next operations. -- Add cancellation, oversized-file, disappearing-directory, ACL, and network-like - slow-I/O tests. - -## Finding 7: agent-filtered search can launch 200 sequential processes without timeout - -Severity: **Medium** - -Implementation status: **Completed on `fix/search-metadata-hardening`.** New -searches supersede the previous request, the whole search has a two-minute -deadline, metadata previews have a 15-second per-item deadline, and no more -than four previews run concurrently across overlapping searches. - -Locations: - -- `src/SkillView.Core/Gh/GhSkillSearchService.cs`, maximum result limit around lines 15-16. -- `src/SkillView.Core/Ui/SkillViewApp.cs`, search state around lines 961-1035. -- `src/SkillView.Core/Ui/SkillViewApp.cs`, metadata filtering around lines 1040-1092. - -### Current behavior - -When an agent filter is supplied, SkillView previews each uncached search result -to read front-matter agent metadata. The configured maximum is 200 results, so -one search can execute 200 sequential `gh skill preview` processes. - -Selected-item preview has a 30-second timeout, but metadata previews use only -the application lifetime token. One stuck command prevents the search from -completing indefinitely. While `_searching` is true, a later user search is -rejected rather than canceling and superseding the stale request. - -### Impact - -- Very high search latency and process churn. -- A single hung preview blocks search until application shutdown. -- Users cannot correct/refine a slow search without quitting. - -### Required remediation - -- Give search its own latest-request gate and deadline. -- Cancel and supersede the previous search when a new one is submitted. -- Apply a timeout to every metadata preview. -- Prefer bulk metadata from `gh` if/when available; otherwise use tightly - bounded concurrency and rate-aware scheduling. -- Preserve the bounded LRU metadata cache. -- Add timeout, supersession, partial-failure, and 200-result tests. - -### Remediation - -`SearchAgentMetadataLoader` now runs at most four preview workers per request -and enforces the same four-slot ceiling across overlapping/superseded searches. -Each metadata subprocess receives a linked 15-second deadline under the outer -two-minute search request. Parent cancellation is propagated; per-item timeout -and transient preview failure affect only that result and remain retryable -rather than being cached as a false empty-agent result. Missing repository -identity is the only non-preview case cached as definitively empty. - -Deterministic coverage holds all four slots while submitting the full 200-result -maximum, proves no fifth preview starts, then releases the batch and verifies -all results. Separate tests cover per-item timeout with a healthy concurrent -result, transient failure retry, and request cancellation reaching every active -preview. The LRU contract test now touches an older entry before eviction and -also verifies that updating an existing value refreshes recency. - -## Finding 8: install modal async handlers can outlive disposed dialogs - -Severity: **Medium** - -Implementation status: **Completed on `fix/final-adversarial-hardening`.** - -Locations: - -- `src/SkillView.Core/Ui/InstallConfirmModal.cs`, async accepting handler around lines 189-264. -- `src/SkillView.Core/Ui/RepoSkillPickerModal.cs`, async accepting handler around lines 249-375. -- `src/SkillView.Core/Ui/InstallScreen.cs`, async accepting handler around lines 394-451. - -### Current behavior - -The Terminal.Gui accepting events use `async void` handlers whose tasks are not -owned or awaited by the modal. `Ctrl+Q` can request stop for both the top modal -and the main window while an install is active. - -The compact and picker handlers check cancellation before calling -`IApplication.Invoke`, but the invocation is queued when called from a worker -thread. They do not recheck the dialog lifetime inside the queued callback. -The dialog can close and be disposed between the check and callback execution. - -An exception thrown by a UI invocation from inside a catch block is outside the -original try/catch and can escape the `async void` handler. - -`InstallScreen` has an inner cancellation check and is safer, but still does not -own/await the operation and can encounter application-disposal races. - -The later disposed-source reassessment adds a second concrete failure mode: -each handler repeatedly reads `lifetime.Token` after awaits even though -`lifetime` is a `using` local owned by the synchronous `Show` method. If the -dialog stops and `Show` returns while its `async void` handler is still active, -the source is disposed and a continuation can throw `ObjectDisposedException` -while merely evaluating the token, including from an exception/cancellation -path. The modal fix must capture a stable token before the first await in -addition to owning and awaiting the operation task. - -### Impact - -- Queued callbacks can update disposed controls. -- Async-void exceptions can reach the application/thread exception path. -- Shutdown may return while install code still owns process and UI references. - -### Required remediation - -- Track the modal's active operation task explicitly. -- Recheck lifetime inside every queued UI action. -- Observe and contain all UI-dispatch exceptions. -- Cancel and await the operation before disposing the dialog. -- Consider an asynchronous modal lifecycle rather than synchronous `Run` plus - untracked event tasks. -- Add Ctrl+Q, Esc, completion-vs-close, and exception-vs-dispose race tests. - -### Remediation - -All three install flows now use `ModalOperationTracker`: one stable token and -one explicitly owned worker for the entire synchronous dialog lifetime. Event -handlers launch owned `Task` work instead of becoming `async void`; queued UI -commits recheck the dialog lifetime inside the callback; dispatch failures are -contained; and disposal cancels and drains the worker before controls are -disposed. Ownership remains non-null through both the worker-complete/UI- -pending window and the smaller UI-commit/worker-return window, so retry, -cancel, close, and escalation shortcuts cannot race a finishing install. - -## Finding 9: CLI cancellation and process termination are incomplete - -Severity: **Medium** - -Implementation status: **Completed on `fix/final-adversarial-hardening`.** - -Locations: - -- `src/SkillView.Core/Bootstrapping/EntryPoint.cs`, `RunAsync` around line 12. -- `src/SkillView.Core/Cli/CliDispatcher.cs`, `RunAsync` around line 23 and service calls throughout. -- `src/SkillView.Core/Subprocess/ProcessRunner.cs`, cancellation handling around lines 68-80. - -### Current behavior - -Neither the shared entrypoint nor CLI dispatcher accepts an application -cancellation token. CLI service calls therefore use default tokens and rely on -abrupt process termination when Ctrl+C is pressed. - -The TUI process runner calls `Kill(entireProcessTree: true)` after cancellation, -but does not wait for the process after issuing the kill. `.NET` process kill -is asynchronous, and parent exit status does not prove all descendants have -exited. - -Most `gh` operations also lack operation-specific deadlines; selected preview -is the notable exception. - -### Impact - -- CLI commands cannot perform cooperative cleanup. -- Long-running/hung `gh` calls have no uniform deadline. -- Cancellation can return before the child process has actually exited. -- Descendants can survive best-effort tree termination. - -### Required remediation - -- Create a root cancellation source in `EntryPoint`. -- Translate `Console.CancelKeyPress` into cancellation and restore/unsubscribe - the handler during teardown. -- Propagate the token through `CliDispatcher` and every service call. -- Apply operation-specific timeouts. -- After `Kill`, perform a bounded wait for parent exit and log/report failures. -- Add cross-platform Ctrl+C and child-process cancellation tests. - -### Remediation - -`EntryPoint` now owns a linked root token, translates Ctrl+C into cooperative -cancellation, unregisters the process-wide handler during teardown, and -returns conventional exit code 130. `CliDispatcher` passes that token through -every probe, inventory, search, preview, install, update, remove, and cleanup -operation. Read-oriented commands have 30-second or two-minute deadlines; -mutating commands have ten-minute deadlines. On cancellation `ProcessRunner` -requests whole-tree termination, waits at most five seconds for the parent, -observes both bounded output drains, and logs kill or wait failure without -hanging shutdown. - -### Post-PR #18 cancellation-admission reassessment - -After the ordered backlog was completed, an analogous-path pass found four -remaining gaps at the *start* of the cancellation lifecycle: - -1. A pre-canceled `EntryPoint` still created logging/services, while a - pre-canceled `CliDispatcher` could print help/version success or run - `doctor --clear-logs` because those synchronous paths never awaited the - propagated token. -2. `ProcessRunner` called `Process.Start` before its first token observation. - An already-canceled caller could therefore launch a child only to kill it, - or receive a start-failure result instead of cancellation. -3. `EnvironmentProbe` scanned every `PATH` entry before reaching a cancellable - subprocess. -4. `SkillViewApp.RunAsync` returned success when cancellation was already - requested (or won during application creation), and could let an active-run - `OperationCanceledException` escape to direct callers. `EntryPoint` masked - part of that contract mismatch, but the awaitable host itself was wrong. - -The new batch closes admission before each boundary. EntryPoint returns 130 -before startup allocation, the dispatcher checks both the root and linked -deadline tokens before routing, log cleanup accepts and checks cancellation, -environment probing checks before and between `PATH` filesystem lookups, and `ProcessRunner` -checks before process configuration/logging/start. The TUI host now returns 130 -for pre-run, creation-time, and active-run external cancellation after its -existing shutdown drain. Tests lock the exit-code constant, prove canceled help -emits no success output, preserve log files on pre-canceled cleanup, use a -nonexistent executable to prove cancellation wins before `Process.Start`, and -exercise real ANSI-driver cancellation through Terminal.Gui teardown. - -### PR #19 post-merge regression-fidelity follow-up - -PR #19's final `Needs a closer look` review identified two tests that could -pass without exercising the boundary named by the test. Both findings were -valid and were selected as the next cohesive batch after merge: - -1. The ANSI integration test used a wall-clock `CancelAfter` before application - creation. A slow worker could therefore cancel during `Init`, take the - creation-time early return, and never enter the active run-loop catch. The - test now schedules cancellation from an ANSI event-loop timeout, asserts - that callback ran, bounds hangs separately with `Task.WaitAsync`, and - observes disposal of the exact created `IApplication` instance. -2. The pre-canceled entrypoint test asserted only exit code 130. It would still - pass if startup allocated the root cancellation handler, logger, sink, and - services before cancellation reached the dispatcher. `EntryPoint` now has - an internal first-resource factory seam; the regression asserts that the - factory is never called for a pre-canceled request. - -The analogous cancellation-test pass found two related assertion gaps. The -pre-canceled environment probe now proves neither `PATH` nor the filesystem is -read, and application-creation cancellation now verifies the created -Terminal.Gui instance is disposed on that early-return path. These are test -fidelity changes, not new production cancellation defects. - -### Post-PR #20 cancellation-callback fault reassessment - -PR #20 received a clean Balanced approval. With the explicit backlog complete, -the next residual-risk pass revisited the earlier rule that cancellation must -run outside ownership locks. That rule prevented lock inversion, but it still -assumed callbacks could not throw. `CancellationTokenSource.Cancel()` invokes -all callbacks and then throws their failures as an `AggregateException`. - -That exception could escape several application-owned boundaries: - -- Replacing a `LatestRequestGate` or `CancellationOperationSlot` published - the new owner before canceling the old one. A throwing old callback prevented - the method from returning the new lease, leaving the published source active - with no owner able to release it. -- `ModalOperationTracker.Dispose` canceled before entering its worker-drain - `try/finally`; a callback failure skipped both the drain and source disposal. -- Cache invalidation, shared-operation final-waiter cleanup, workspace/modal - teardown, and Ctrl+C propagation could return or unwind before their required - cleanup completed. -- `CancelAfter` initiated cancellation on a timer thread with no caller able to - observe or contain callback exceptions, turning a bad operation callback into - a process-level failure. - -The new `SkillView.Threading.CancellationSource` is the single owner for manual, -parent-linked, and deadline cancellation. It captures aggregate callback -failures, isolates best-effort diagnostics, returns a stable token, and defers -resource disposal while any cancellation call is active. Root, CLI, subprocess, -search metadata, request gates, operation slots, shared flights, modal and -workspace lifetimes, and cleanup/remove dialogs now use that owner; no raw -production `CancellationTokenSource` or `CancelAfter` remains outside it. - -Deterministic regressions cover parent and timer initiation, a diagnostic -callback that also throws, self-disposal from a cancellation callback, -cancel/dispose contention, replacement ownership, cache invalidation, shared -operation cleanup, modal worker drain, workspace navigation, and metadata -preview timeout. Callback faults are contained without weakening cancellation -or allowing required cleanup to finish early. - -### Claude review follow-up on PR #21 - -An independent Claude Code review stress-tested the new owner and confirmed its -cancel/dispose interleavings. Its nine concrete findings were accepted: - -1. Root cancellation, preview/search gates, Installed/Changes/Updates operation - slots, the environment-probe flight, and `gh skill list` cache flights now - supply callback reporters instead of silently containing faults. -2. `SkillViewApp`'s constructor-owned request gates and shared probe are assigned - where their logger-backed reporters are available; the earlier unexplained - field-initializer move is now purposeful. -3. The unreachable `ObjectDisposedException` catch was removed. The active- - cancellation count prevents the owned source from being disposed during - `_source.Cancel()`. -4. `Cancel()` after `Dispose()` is documented and tested as an intentional - no-op. Disposal closes admission and leaves an uncanceled stable token - uncanceled. -5. Deadline timer creation suppresses `ExecutionContext` flow, preventing - command and operation deadlines from retaining ambient `AsyncLocal` or - `Activity` state for up to ten minutes. -6. Callback reports include the full flattened aggregate rather than only - `AggregateException.Message`, preserving exception types, inner messages, - and stacks within the logger's existing character budget. -7. A timeout-only constructor removes the artificial - `CancellationToken.None` argument from subprocess termination waits. -8. Construction now documents why an already-canceled parent's synchronous - registration callback is safe before `_parentRegistrations` is assigned. -9. `CancellationTokenSourceSlot` was renamed to `CancellationOperationSlot` to - describe the abstraction it now owns. - -The review's logging-factory simplification was also accepted as -`CancellationCallbackReporter`, which keeps categories and full diagnostic -formatting consistent. Two broader simplifications were rejected after tracing -their resource and ordering consequences. `CancellationSource` continues to -dispose its underlying CTS because disposal releases token callback -registrations and any lazily-created wait handle; intentionally leaving it -undisposed would weaken the repository's resource-lifetime contract for little -code reduction. `LatestRequestGate` and `CancellationOperationSlot` retain -their outer cancel-in-progress state because it does more than protect -`CancellationSource.TryCancel()` from disposal: it guarantees cancellation wins -when replacement races the superseded lease's release. Removing that state -would allow release to dispose the source first, after which the documented -post-dispose `TryCancel()` refusal could leave the superseded token uncanceled. - -New regressions assert both containment and reporting on every previously -silent owner family, the post-dispose contract, and absence of ambient -`ExecutionContext` on the deadline timer thread. - -### Post-PR #21 cancellation-owner lifecycle follow-up - -With the explicit SkillView backlog complete, the next residual-risk pass -revisited construction and disposal of the new cancellation owner. It found -three related resource-lifetime gaps selected as the next batch: - -1. `Dispose()` closed cancellation only after `_resourcesDisposed` became true. - When disposal raced a callback already in progress, `_disposeRequested` was - true but resources correctly remained live until that callback returned. A - second cancellation request could therefore enter after disposal, contrary - to the documented admission contract and `TryGetActiveToken`. The underlying - source was already canceled, so this repeated request did not rerun callbacks - or change user-visible cancellation behavior. `TryCancel()` now reports - admission and rejects new calls as soon as disposal is requested. A - deterministic regression holds the first callback open across disposal and - proves the second request is refused. -2. The timeout check covered only non-positive values. `Timer` also rejects - values above `uint.MaxValue - 1` milliseconds; discovering that after parent - registration could leave a partially constructed owner reachable from the - parent token. The complete timer range is now validated before any resource - acquisition. -3. Construction now rolls back parent registrations if linking fails. Every - callback-visible field is assigned before the deadline timer is armed; a - timer creation/arming failure closes admission through normal deferred - disposal instead of bypassing active-callback accounting. If parent - cancellation wins during construction, registration stops and no needless - deadline timer is retained for an owner that is already canceled. - -### Independent Claude review of PR #22 - -An independent Claude Code Opus review confirmed the concurrency behavior with -approximately 1,900 barrier-synchronized construction, parent, deadline, -cancellation, disposal, and throwing-callback races. Its recommendations were -reassessed as follows: - -1. **Canceled-parent timer retention regression — accepted.** A five-minute - deadline previously rooted an abandoned owner even when its parent was - already canceled. A serialized `WeakReference` regression now proves the - owner is collectible immediately; a revert that recreates the timer fails. -2. **Admission wording — accepted.** The repeat cancellation was an underlying - CTS no-op, not a user-visible shutdown correction. The audit and PR describe - it as contract consistency between disposal, `TryGetActiveToken`, and the - renamed `TryCancel` boundary. -3. **Redundant `_resourcesDisposed` state — accepted.** Closing admission with - `_disposeRequested` makes the call that observes the active count reach zero - the unique physical disposer. The third flag and dead conjuncts were removed, - with that uniqueness invariant documented at the transition. -4. **Construction rollback protocol — accepted and strengthened.** Parent - registrations use an honest post-registration count. The deadline timer is - created disabled only after parent fields are assigned, then armed. A timer - creation or arming failure calls normal `Dispose`, so admitted callbacks - drain before physical resources are released. -5. **Fractional upper timer bound — not adopted.** SkillView intentionally uses - a conservative `TimeSpan` bound rather than depending on `Timer` truncating a - fractional millisecond at the approximately 49.7-day maximum. No production - deadline approaches that boundary. -6. **Injected metadata timeout validation — accepted.** The loader validates - through the cancellation owner's shared range predicate at construction, so - an oversized injected timeout cannot fault a whole search from inside a - per-result worker. -7. **Overstated test name — accepted.** The range test no longer claims to - observe resource creation; the new retention regression covers the actual - resource behavior. -8. **Cancellation naming — accepted.** The owner method is now `TryCancel`, - whose Boolean means admission was open, while request/operation gates retain - `Cancel`, whose Boolean means an active owner existed. The cache's void - `TryCancel` wrapper was removed. - -## Finding 10: removal materializes full trees and runs on the UI thread - -Severity: **Medium** - -Implementation status: **Completed on `fix/removal-lifecycle-hardening`. The -portable path remains non-atomic against hostile same-user path replacement, -as documented under Finding 1.** - -Locations: - -- `src/SkillView.Core/Inventory/RemoveService.cs`, `.ToList()` traversal around lines 106-121. -- `src/SkillView.Core/Ui/RemoveConfirmModal.cs`, synchronous remove around line 139. -- `src/SkillView.Core/Ui/RemoveScreen.cs`, synchronous execute around lines 268-287. -- `src/SkillView.Core/Ui/CleanupScreen.cs`, synchronous batch removal around lines 179-241. - -### Current behavior - -The original implementation materialized every file path and directory path -before deletion and ran synchronously from Terminal.Gui event handlers. The -earlier branch replaced that traversal with O(depth) enumerator frames. This -branch completes the remediation: compact remove, advanced remove, cleanup -batches, cleanup validation, and agent-link unlinking run off the UI thread; -Esc cancels active work; progress is throttled to 10 updates per second; and -each owning window cancels and drains its task before disposing controls. - -Cancellation publishes exact aggregate progress, including cancellation -between batch targets. The workflow invalidates and rescans inventory whenever -even a partial target deleted files or directories. Runtime failure detail is -bounded to 128 messages plus an omission summary while `ErrorCount` preserves -the exact total. PR #14's first Copilot review found two terminal-progress -gaps: already-canceled link removal omitted its final canceled event, and the -outer batch catch could overwrite exact mid-target counts with completed-target -totals. Both are fixed by explicit early-cancellation publication and by making -the batch adapter retain and republish its latest aggregate snapshot. - -PR #14's Balanced follow-up review surfaced six suppressed accounting issues -in the same change set. All were accepted and fixed in the PR: progress now -distinguishes processed targets from successfully deleted targets; compact and -advanced removal flows retain mutation totals across retries and escalation; -cancellation reports preserve the exact observed runtime-error count while -marking cancellation separately; and remove JSON only emits runtime-error -fields after validation and confirmation permit an actual removal or dry run. -The subsequent Balanced review also found that a refused direct removal -returned after its initial progress event. Refusals now publish a forced -terminal completed snapshot with one processed target, zero deleted targets, -and the refusal error count. - -PR #14's final post-merge review contained one suppressed finding from -unchanged lines. It was independently reassessed and accepted: - -1. **Compact-modal shortcuts can race a queued completion callback — correct - and fixed on `fix/search-metadata-hardening`.** `Task.IsCompleted` can become - true on a worker thread before the callback queued through - `IApplication.Invoke` commits `Removed`/`Failed` and clears or closes the - modal. During that interval, Esc/`n` could overwrite a successful outcome - with `Cancelled`, while `a` could open the advanced wizard against an - already-deleted skill. Any non-null task is now treated as owned until its - UI completion runs. Esc cancels only a still-running worker; completed-but- - queued workers display a finishing status and all close/escalate shortcuts - remain blocked. A state-classification regression test protects the worker- - complete/UI-pending boundary. - -### Impact - -- Large trees allocate proportional path lists before deletion begins. -- Slow local disks, antivirus, network mounts, or large packages freeze the UI. -- Users cannot cancel after confirming. -- Link cycles amplify both traversal time and memory until failure. - -### Required remediation - -- First apply the critical reparse-point fix. -- Replace complete file/dir materialization with an explicit bounded traversal. -- Make removal asynchronous and cancellation-aware. -- Report progress through throttled UI dispatch. -- Preserve bottom-up directory deletion without retaining every file path. -- Add large-tree, slow-I/O, cancellation, and partial-failure tests. - -## Finding 11: path identity is wrong on case-insensitive filesystems - -Severity: **Medium** - -Implementation status: **Completed on `fix/final-adversarial-hardening`.** - -Locations: - -- `src/SkillView.Core/Inventory/PathResolver.cs`, containment around lines 69-79. -- `src/SkillView.Core/Inventory/ScanRootResolver.cs`, root deduplication around lines 43-46. -- `src/SkillView.Core/Inventory/LocalInventoryService.cs`, merge indexes. -- Other cleanup and removal sets keyed by normalized path with `StringComparer.Ordinal`. - -### Current behavior - -Path normalization changes separators and trims trailing separators, but it -does not canonicalize case. Path equality, containment, deduplication, and merge -keys use ordinal case-sensitive comparisons. - -On normal Windows filesystems, `C:\Users\X` and `c:\users\x` identify the same -path but SkillView treats them as unrelated. Case-insensitive macOS volumes have -the same class of problem. Linux filesystems are normally case-sensitive, and -Windows can enable case sensitivity per directory, so an unconditional global -ignore-case comparer is also not a complete solution. - -### Impact - -- Legitimate containment validation can fail. -- The same physical root or skill can appear more than once. -- `gh skill list` records may not merge with filesystem records. -- Cleanup classification can report false duplicates or anomalies. - -### Required remediation - -- Centralize path identity rather than selecting comparers independently. -- At minimum use OS-appropriate ordinal comparison consistently. -- Where destructive identity matters, prefer canonical filesystem identity or - independently revalidated containment rather than string equality alone. -- Add Windows mixed-case, macOS case-insensitive-volume, Linux case-sensitive, - and Windows case-sensitive-directory tests. - -### Remediation - -`PathIdentity` is now the single normalization, equality, keying, and -containment boundary. It detects case behavior from an existing filesystem -entry instead of assuming every macOS or Windows volume is insensitive, while -preserving case conservatively when probing is impossible. Inventory merge and -deduplication, cleanup/removal indexes, CLI inventory diffs, scan-root -deduplication, lock-file tracking, and active log-file exclusion use the same -keys or equality rule. Probes use targeted case-sensitive enumeration rather -than materializing parent directories, and root paths retain their separator -so `/` and `C:/` containment remain correct. - -## Finding 12: Discover and Doctor operations outlive workspace ownership - -Severity: **High** - -Implementation status: **Completed on `fix/adversarial-hardening`.** - -Locations: - -- `src/SkillView.Core/Ui/SkillViewApp.cs`, `ActivateTab` and `EnterDoctor`. -- `src/SkillView.Core/Ui/SkillViewApp.cs`, `RunSearchAsync` around lines 967-1044. -- `src/SkillView.Core/Ui/SkillViewApp.cs`, `PreviewSelectedAsync` around lines 1100-1223. -- `src/SkillView.Core/Ui/SkillViewApp.cs`, Doctor probing around lines 764-800. - -### Current behavior - -The post-PR shared cancellation helper covers Installed, Changes, and Updates. -It does not cancel work owned by Discover or Doctor: - -- Discover search uses only the application lifetime. Switching to Installed, - Changes, or Doctor leaves the search and up to 200 agent-metadata preview - subprocesses running. A late success replaces hidden Discover state, calls - `SetFocus` on the results table, changes the global status, and clears the - shared spinner. -- Selected preview has its own latest-request gate, but leaving Discover does - not cancel it. Its success callback calls `ShowPreviewPane`, which can close - a log pane the user opened while preview was in flight and changes global - status/spinner state after another workspace became active. -- Starting a new search does not cancel an in-flight selected preview for the - old result set. The old preview can therefore overwrite the preview/title - after the new search results have been installed. -- Doctor probing uses the application lifetime and `RunBackground`. Leaving - Doctor before the probe completes does not cancel or supersede it. The late - callback updates the hidden Doctor view and clears the shared spinner. -- Startup, Doctor, and coordinator paths can all observe `_lastReport == null` - and launch duplicate environment probes rather than sharing one in-flight - task. - -Terminal.Gui currently rejects `SetFocus()` when a view or ancestor is hidden, -so the hidden-focus calls are less damaging than they appear. The remaining -status, spinner, preview-mode, process, and hidden-state mutations are still -real application-level lifecycle defects. - -### Impact - -- Hidden work consumes `gh` processes, filesystem I/O, and memory after the - user has left the feature. -- One workspace can clear or overwrite another workspace's progress and status. -- A selected preview from the old result set can be displayed beside new search - results. -- Returning from logs or another tab can reveal state chosen by a stale - completion rather than the user's latest action. - -### Required remediation - -- Give every workspace an explicit activation lifetime, separate from the - application lifetime. -- Cancel Discover search and selected preview when Discover deactivates. -- Cancel or generation-gate Doctor probing when Doctor deactivates. -- Include workspace/generation checks inside the queued UI callback, not only - before calling `Invoke`. -- Separate progress ownership so one operation cannot clear another operation's - spinner/status. -- Store and share one in-flight environment-probe task. -- Add transition tests for Discover to every primary tab, Discover to Doctor, - preview to logs, new-search versus old-preview completion, and Doctor leave - versus probe completion. - -## Finding 13: log snapshot/replay initialization can lose or duplicate entries - -Severity: **Medium** - -Implementation status: **Completed on `fix/adversarial-hardening`.** - -Locations: - -- `src/SkillView.Core/Ui/SkillViewApp.cs`, `OnLogEntry` and - `InitializeVisibleLogLines` around lines 1909-1941. -- `src/SkillView.Core/Logging/FileLogSink.cs`, `Attach` around lines 44-55. -- `src/SkillView.Core/Logging/Logger.cs`, separate ring and observer locks. - -### New Copilot "needs a closer look" comment - -Copilot's -[post-fix review](https://github.com/harder/gh-skillview/pull/11#pullrequestreview-5052520853) -correctly identified this visible-log loss interleaving: - -1. `InitializeVisibleLogLines` takes `Logger.Snapshot()`. -2. A new log entry is added and `OnLogEntry` enqueues it. -3. Initialization acquires `_visibleLogGate`, clears the queue, and replaces it - with the older snapshot. - -The new entry is permanently absent from the visible log pane until the pane is -closed and initialized again. - -The suggested minimal change—hold `_visibleLogGate` while taking the logger -snapshot—prevents that loss but needs closer design. `Logger.Log` commits the -ring entry before it invokes observers. A log thread can add an entry, block in -`OnLogEntry` on `_visibleLogGate`, and then have initialization include that -same entry in the snapshot. When the gate is released, the callback enqueues it -again, producing a duplicate. - -`_showingLogs` is also written on the UI thread and read by logger-callback -threads without a lock or volatile access. The callback can observe stale pane -visibility while deciding whether to retain and schedule an entry. - -### Analogous file-sink race found during reassessment - -`FileLogSink.Attach` replays `logger.Snapshot()` and subscribes afterward. A log -emitted between those calls is in neither the replay nor the new subscription, -so it is absent from the disk log. Reversing the order to subscribe first and -snapshot second creates a duplicate window instead of closing the race. - -The public `Attach` method can also race re-attach or disposal; its one-shot -startup assumption is not expressed or enforced by the API. - -### Required remediation - -- Define a logger sequence number or an atomic replay-subscribe operation. -- Commit ring insertion and observer membership against one ordering boundary, - then deliver replay/live entries with a watermark so each sequence appears - exactly once. -- Use the same primitive for the file sink and visible log pane rather than - maintaining two subtly different replay protocols. -- Synchronize pane visibility with queue state, or enqueue all bounded entries - regardless of visibility and only coalesce drawing while visible. -- Make file-sink attachment one-shot and startup-only, or fully synchronize - attach/re-attach/dispose. -- Add deterministic tests for logs emitted before snapshot, between snapshot - and subscription/replacement, and after subscription, asserting no gaps and - no duplicates. - -## Finding 14: Esc does not leave all Discover filter inputs as advertised - -Severity: **Medium usability/reliability** - -Implementation status: **Completed on `fix/final-adversarial-hardening`.** - -Location: - -- `src/SkillView.Core/Ui/SkillViewApp.cs`, root Esc handling around lines 598-607. - -### Current behavior - -Copilot also recorded this in the -[preceding review's suppressed follow-ups](https://github.com/harder/gh-skillview/pull/11#pullrequestreview-5052291512). -The query field handles Esc locally by returning focus to the results table. -The owner field, agent field, and numeric limit control do not. Their Esc key -reaches the root handler, which consumes it and displays "Esc leaves the field" -without actually changing focus. The next plain `q` is consequently typed into -the field rather than quitting. `Ctrl+Q` remains the unconditional escape hatch, -but the UI message and expected two-step Esc then q behavior are wrong. - -### Required remediation - -- Centralize `LeaveTextInput` and call it for every editable Discover/Installed - control before consuming Esc. -- Test query, owner, agent, numeric limit, and Installed filter focus separately. -- Keep the application-level Ctrl+Q test because focused editors are precisely - where global quit routing tends to regress. - -### Remediation - -The main shell now routes Esc through one `LeaveTextInput` action: Discover -query focus returns to results and Installed filter focus returns to the skill -list without clearing text. Owner, agent, and limit moved into the compact -Discover filters dialog in the intervening UI redesign; Esc in each editor now -moves focus to Cancel and explicitly advertises that a second Esc closes the -dialog. `Ctrl+Q` remains unconditional. - -## Test gap: the LRU test previously proved only FIFO eviction - -Status: **Closed on `fix/search-metadata-hardening`.** - -`Store_EvictsLeastRecentlyUsedMetadataAtCapacity` now touches `first` before -storing `third` and asserts that `second` was evicted. A separate test verifies -that updating an existing value refreshes recency and retains the replacement -metadata. - -## Lower-risk hardening opportunities - -These should be included while the related components are being changed: - -1. [x] Validate that `Logger` capacity is non-negative; completed in PR #12. -2. [x] Serialize tests that mutate `TuiHelpers.CurrentTheme`, Terminal.Gui - scheme facades, or other static UI configuration; completed on - `fix/final-adversarial-hardening` with a nonparallel static-state collection. -3. [x] Stream CLI JSON directly to `Console.Out` rather than creating a - `MemoryStream`, copying it to a byte array, decoding to UTF-16, and encoding - it again for output. Completed with a non-owning UTF-8 `TextWriter` stream; - snapshot-only render helpers retain their in-memory path. -4. [x] Add explicit concurrency coverage for: - - Cache get/store/invalidate. - - Logger sink append/dispose. - - Logger replay/subscribe without gaps or duplicates. - - UI shutdown with pending startup/search/rescan tasks. - - Every workspace exit with pending load/operation/UI dispatch. - - Modal shutdown with active installs. - - Same-day log growth and active-file rotation. - Existing suites already covered the logger, cache, workspace, and shutdown - interleavings; this batch adds install ownership and metadata-cache - contention tests. -5. [x] Add stress tests with bounded memory assertions for noisy subprocess - errors, oversized local files, large inventories, and large removal trees. - -## Existing hardening that held up - -The following areas were reviewed and found to be appropriately bounded or -coordinated within their current scope: - -- `LatestRequestGate` correctly supersedes selected previews and applies their timeout. -- Installed and Changes cancellation slots prevent stale loads from replacing - newer state. Updates now does the same for both loads and operations after - `df62a22`; this was not true at the original audit commit. -- `ProcessRunner` drains stdout and stderr concurrently and bounds each captured stream. -- Search agent metadata uses a locked, capacity-limited LRU. -- Visible-log redraws are coalesced; this branch adds both line-count and total - character budgets while retaining entries regardless of pane visibility. -- Terminal escape sanitization is stateless and applied to remote/untrusted - rendered content. -- The application uses Terminal.Gui's modern `Application.Create().Init()` and - `IApplication.Dispose()` lifecycle. -- External TUI cancellation is connected to `IApplication.RunAsync`; this - branch adds ownership and shutdown quiescence for subordinate application - tasks, while modal-specific lifetimes remain remediation item 10. - -## Recommended remediation order - -1. [x] Fix static nested-link traversal before any further release or removal testing. -2. [x] Make `GhSkillListCache` thread-safe and single-flight. -3. [x] Add tracked background-task ownership and shutdown quiescence. -4. [x] Add workspace activation lifetimes for Discover and Doctor, including - search/preview/probe cancellation and generation checks. -5. [x] Replace log snapshot-plus-subscribe/replacement with exact-once replay. -6. [x] Add log character/byte budgets and correct disk rotation. -7. [x] Enforce aggregate disk retention during active-file growth and remove - cancellation-callback execution from request/slot ownership locks. -8. [x] Move inventory and removal I/O off the UI thread with cancellation, then - implement the native removal boundary for hostile path replacement. Windows - deletion is opened-object-bound. Unix traversal is descriptor-relative and - identity-checked; its irreducible final-entry name interval is - documented above rather than represented as fully atomic. -9. [x] Finish metadata-preview deadlines and bounded scheduling. Search - supersession, the whole-request deadline, a per-preview deadline, and a - global four-process ceiling are complete. -10. [x] Make modal operation lifetimes awaitable and disposal-safe, including - stable token capture before the first await and both queued-completion - ownership boundaries. -11. [x] Wire root CLI cancellation, operation-specific deadlines, exit code - 130, and bounded post-kill waiting. -12. [x] Centralize volume-aware cross-platform path identity semantics. -13. [x] Correct Esc focus behavior and strengthen the LRU contract test. -14. [x] Finish the lower-risk hardening and stress coverage. - -`FileLogSink` lock ordering, Doctor-to-tab cancellation, and Updates operation -deactivation were completed in `df62a22` and are not in the remaining order. - -### Final portable-hardening checkpoint - -Local verification for `fix/final-adversarial-hardening` completed with a -zero-error build, all 679 unit and ANSI-driver integration tests, and native -macOS ARM64 AOT publishes for both the standalone app and gh extension. New -tests cover install worker/UI ownership boundaries, cancellation propagation, -bounded child-process termination, volume-observed path semantics and root -containment, Discover/Installed Esc behavior, split UTF-8 streaming, concurrent -metadata-cache use, and allocation bounds for noisy output, oversized skills, -500-skill inventories, and 2,000-file removals. - -### PR #16 Copilot review disposition - -The Balanced review generated four comments. All four were independently -reassessed and accepted: - -1. **Compact-install cancellation stranded ownership — correct and fixed.** - The cancellation catch logged and returned without a terminal UI commit, - leaving the completed worker owned forever. It now queues the same guarded - spinner reset, canceled outcome, and modal close used by the other install - flows. -2. **Terminal UI-dispatch failure stranded ownership — correct and fixed.** - Logging a dispatch/callback exception was insufficient when the callback - was responsible for release or close. Terminal commits now use a distinct - tracker API that requests release on either failure path, but interim picker - progress deliberately retains ownership if its callback fails. -3. **CLI cleanup classification ignored cancellation — correct and fixed.** - Inventory capture received the root/deadline token, but the immediately - following classifier used its non-cancellable wrapper. CLI cleanup now calls - `ClassifyWithCancellation` with the same token. -4. **Missing-path probing measured the wrong Windows directory — correct and - fixed.** For a missing child, climbing upward and flipping the existing - parent's name measured the grandparent's lookup behavior even though - Windows case sensitivity is per directory. Missing direct children now - probe another entry in their actual containing directory; missing parents, - empty directories, and failed probes fall back conservatively to preserving - case rather than collapsing potentially distinct paths. - -Follow-up validation passed all 683 unit and ANSI-driver integration tests and -both macOS ARM64 native AOT publishes. - -## Terminal.Gui upstream assessment - -Most SkillView findings belong in SkillView rather than Terminal.Gui. The -library cannot know that hiding Discover should cancel a `gh` process, and it -does not own SkillView's logger, filesystem traversal, path identity, cache, -or process-output policy. Terminal.Gui already rejects `SetFocus()` when a view -or an ancestor is hidden, removes `SpinnerView` timers during disposal, exposes -application-level keyboard routing, and provides `Markdown.ShowHeadingPrefix`. - -The audit did identify the following upstream defects or worthwhile platform -improvements. They were checked against the current `develop` source at -`48efa0c5` and stable `v2.4.17`. - -### TG-1: `TimedEvents.RunTimers` still executes callbacks under its queue lock - -Priority: **High; new upstream issue recommended** - -`TimedEvents.RunTimers` takes `_timeoutsLockToken` and calls `RunTimersImpl`. -`RunTimersImpl` takes the same re-entrant lock, removes one timeout, exits only -the inner lock, and invokes the user callback. Despite its comment saying the -callback executes outside the lock, the outer `RunTimers` lock remains held. -This exists in both stable `v2.4.17` and current `develop`. - -A timeout callback that waits for background work which calls `AddTimeout`, -`RemoveTimeout`, or `IApplication.Invoke` can deadlock: the UI callback waits -for the worker, while the worker waits for the timeout lock held by the UI -callback. Long callbacks also block all cross-thread UI dispatch and timer -cancellation. - -Do not simply remove all serialization because `RunTimers` is public and two -callers could then execute callbacks concurrently. Use a separate runner gate -or interlocked single-runner guard, while retaining the short queue lock only -for selecting/removing/reinserting entries. Add a deterministic test whose -callback waits for a background `Add`/`Remove`; it must complete without either -operation running concurrently with another timer callback. - -No existing issue located by `TimedEvents`, `RunTimers`, lock, or deadlock -search describes this exact retained outer-lock problem. - -### TG-2: complete the synchronization-context session fix before v2.5 - -Priority: **High; already tracked** - -Stable `v2.4.17` installs a plain `SynchronizationContext`, so `await` in an -event handler resumes on the thread pool. The current multitasking guide says -the continuation automatically returns to the main thread, which is not true -for the latest stable release. Issue -[tui-cs/Terminal.Gui#5579](https://github.com/tui-cs/Terminal.Gui/issues/5579) -and PR -[tui-cs/Terminal.Gui#5588](https://github.com/tui-cs/Terminal.Gui/pull/5588) -introduced a real `MainLoopSyncContext` on `develop`. - -That change created the startup/session deadlock tracked by -[tui-cs/Terminal.Gui#5636](https://github.com/tui-cs/Terminal.Gui/issues/5636). -PR -[tui-cs/Terminal.Gui#5641](https://github.com/tui-cs/Terminal.Gui/pull/5641) -contains the session-scoping and after-session fallback, but it was merged into -a stacked feature branch rather than current `develop`; the issue remains open. -Treat integration of that fix, including nested-session tests, as a v2.5 -release gate. Version the multitasking documentation or clearly describe the -different stable and upcoming behavior until then. - -### TG-3: the deeper `IApplication.Invoke` lifecycle race remains unresolved - -Priority: **High; new or reopened upstream issue recommended** - -[Issue #5163](https://github.com/tui-cs/Terminal.Gui/issues/5163) identified -that the fast path reads `TopRunnableView` and `MainThreadId` without -synchronizing against shutdown. PR -[#5185](https://github.com/tui-cs/Terminal.Gui/pull/5185) added a useful -pre-init/post-dispose `Initialized` guard, but explicitly left the deeper race -out of scope while closing the issue. The current implementation still permits -state to change after the guard or fast-path check and before action invocation -or timeout enqueue. - -Re-open the concurrency portion as its own issue. Define whether a concurrent -shutdown causes the action to run, be canceled, or throw, then make the -decision atomic. Cover both the immediate UI-thread path and queued worker path -with deterministic lifecycle hooks rather than timing-only stress tests. - -### TG-4: add awaitable, cancellable, session-aware UI dispatch - -Priority: **Medium API improvement** - -`IApplication.Invoke` returns `void`; callers cannot await execution, attach a -cancellation token, distinguish "queued" from "ran", or bind the callback to -the `IRunnable`/modal that owns the referenced views. SkillView had to build a -`TaskCompletionSource` wrapper and still implement all view-lifetime checks -itself. - -Consider an `InvokeAsync(Action, CancellationToken)` API and a runnable/session -lifetime token that is canceled when that session ends. The callback should -recheck the token on the UI thread immediately before invoking user code. -Document how it behaves after session end and application disposal. This would -not automatically cancel application business operations, but it would make -safe modal/workspace dispatch a standard pattern. - -The current multitasking guide and UICatalog threading scenario rely heavily on -`async void` event handlers and show cancellation without tying it to -`Disposing` or `IRunnable.IsRunningChanged`. Add an explicit async-modal example -that cancels and observes work before disposing referenced views. Longer term, -an async command/event abstraction could observe handler tasks and route their -exceptions through the application error handler. - -### TG-5: add debug-time UI-thread/lifetime diagnostics - -Priority: **Medium diagnostic improvement** - -Terminal.Gui documents off-thread view mutation as undefined behavior, but most -view property mutations do not assert thread affinity. An opt-in debug guard or -an analyzer could flag mutation when `View.App.MainThreadId` differs, and warn -when an async event handler captures a view without cancellation on disposal. -This would have surfaced several SkillView audit findings much earlier without -adding release-build overhead. - -## Primary platform references - -- .NET recursive search and reparse points: - -- .NET directory enumeration: - -- .NET process termination: - -- .NET console cancellation: - -- Windows file-sharing/delete behavior: - -- Windows `FILE_DISPOSITION_INFO` native layout: - -- Windows handle-based disposition API: - -- Windows/Linux case sensitivity: - - -## 2026-08-31 PR #17 comprehensive follow-up - -An independent Claude Opus review of commit `8300606` reported ten additional -issues. All ten were traced through their production callers rather than -accepted from the report at face value. - -1. **macOS x64 native ABI — valid, fixed fail-closed.** Apple's x86_64 headers - map the unsuffixed `fstat`, `fstatat`, and `readdir` symbols to the legacy - 32-bit-inode ABI, while SkillView parses the 64-bit-inode layout. Secure Unix - removal now supports macOS only in a native little-endian ARM64 process and - throws/refuses rather than interpreting unverified bytes on Intel or under - Rosetta. This is preferable to adding a second ABI in the security boundary; - releases currently ship `osx-arm64`. -2. **Unavailable backend also blocked dry-run — valid, fixed.** Real removal - remains fail-closed and now reports the backend reason as an environment - failure. A non-mutating `remove` preview may use managed canonicalization and - policy inspection when the backend is unavailable, but returns no execution - identity. `RemoveService` still refuses every real directory or link delete - without its native identity. Warning confirmation is required only for - execution, not for preview output. -3. **CliList-only agent links outside roots — valid UX gap, fixed without - weakening policy.** Such links remain blocked, and the wizard explains that - the directory must be supplied with `--scan-root`. SkillView deliberately - does not promote a path reported by `gh skill list` into a trusted removal - root automatically; doing so would let discovery data redefine the deletion - authority boundary. -4. **advanced-remove validation on the UI thread — valid, fixed.** Initial and - selection-specific evaluations now run in a modal-owned background - operation. Per-target results are cached only for display. Finish performs a - fresh background validation and returns to Review if its blocking errors, - warnings, resolved paths, or incoming-link content changed; only the fresh - native identities reach execution. -5. **Windows directory-entry header bound — valid defence in depth, fixed.** - Enumeration verifies that the full 88-byte fixed header remains before any - field read and rejects offsets that cannot leave another complete header. -6. **Unix `EINTR` handling — valid reliability gap, fixed.** Open, openat, - openat2, fstat, fstatat, dup, readdir, and unlinkat use bounded retries. - Every unlink retry rechecks cancellation because it is a new destructive - call. `close` and `closedir` are not retried. -7. **public root-unvalidated `RemoveLinkAsync` — valid, removed.** Agent-link - deletion now has one route: `ValidateSymlink` followed by `RemoveAsync`. - Managed path deletion branches were removed; the remaining managed traversal - is explicitly preview-only. -8. **dead `FindContainingRoot(canonicalizeRoots:)` branch — valid, removed.** - The helper now performs its only real raw-path containment role; canonical - containment remains part of the held-root identity capture. -9. **cleanup duplicate accounting — valid, fixed.** `RemoveMany` logs and counts - canonical duplicates as skipped. Cleanup subtracts skipped targets before - calculating failures, so a safe deduplication is not mislabeled as a failed - removal. -10. **`openat2` probe depended on CWD — valid, fixed with a qualification.** - Probing absolute `/` with `RESOLVE_BENEATH` would fail by contract. SkillView - instead opens `/` once and probes `.` relative to that descriptor using the - exact production resolve flags. Platform support is cached, and an - unavailable Linux backend includes the probe errno and message in its - diagnostic. - -Targeted regression coverage now includes the macOS architecture matrix, -bounded `EINTR` retry, Windows fixed-header bounds, unsupported-backend preview -versus execution behavior, outside-root agent-link messaging, and duplicate -batch accounting. The required verification matrix remains full unit and -integration tests, macOS/Linux/Windows CI, CodeQL, and sequential Native AOT -publishes for both hosts. - -### Copilot follow-up on the comprehensive fixes - -The next balanced review identified two gaps in the preceding changes. Both -were valid after tracing them through the complete workflow: - -1. **Finish-time validation could authorize a replacement object.** Comparing - only paths, errors, warnings, and incoming links meant a directory or link - replaced after Review could receive a new native identity and still inherit - the user's earlier confirmation. `HasSameSafetyContract` now compares the - full directory or parent/link execution identity plus the link-only and - empty-directory execution modes. Any authority change returns the wizard to - Review; the replacement's fresh identity is never executed under the old - confirmation. -2. **Duplicate cleanup candidates could fail before batch deduplication.** A - path may appear under more than one cleanup classification. The lazy - sequence previously validated and removed the first candidate before seeing - the second, so the second failed validation instead of reaching - `RemoveMany`'s canonical deduplicator. Cleanup now resolves all selected - path keys and removes duplicates before yielding the first validation, then - adds those skips to the batch report. Unique candidates remain lazily pinned - immediately before their individual removals. - -### Copilot follow-up on held-root mount boundaries and canceled accounting - -The next balanced review exposed three related omissions. All three were valid: - -1. **Selected Unix directories could cross a mount below the held scan root.** - Validation opened each relative component with ordinary `openat`, so a Linux - bind mount or cross-device macOS directory could become the captured removal - baseline before deletion-time descent checks ran. Root-relative validation - now uses Linux `openat2(RESOLVE_NO_XDEV)` for every component and verifies - every opened component's device against the held root on both Unix platforms. -2. **Link parents had the same mount escape.** Broken-link cleanup and agent - unlink validation now apply the same no-cross-device component walk before - issuing `ExecutionLinkIdentity`; a link inside an external mounted tree is - refused before it can be authorized for `unlinkat`. -3. **Canceled TUI cleanup lost known duplicate skips.** The skip count existed - only inside the async iterator/report path. If cancellation threw before a - report returned, the outer status recomputed every non-deleted selection as a - failure. Per-attempt state now publishes pre-validation skips before removal - begins and carries them into cancellation accounting. - -Regression coverage uses Linux's `/proc` mount to prove both directory and link- -parent validation refuse a mounted subtree, directly checks the macOS/Linux -device guard, and cancels a duplicate cleanup selection at the native deletion -boundary to prove its skip remains observable. - -### PR #17 final review finding and post-merge backlog disposition - -Copilot's final `Needs a closer look` review -([review 5071638437](https://github.com/harder/gh-skillview/pull/17#pullrequestreview-5071638437)) -identified one previously missed UI-thread path. It was accepted into the -post-merge backlog at highest priority and selected as the next coherent batch: - -1. [x] **Move compact-remove routing off the Terminal.Gui thread.** - `SkillViewWorkflowCoordinator.OpenRemoveDialogInternal` built removal targets - and called `RemoveTargetResolver.Evaluate` directly from the Installed tab's - `onRemove` callback. That evaluation can open native handles, enumerate the - selected directory for policy, and inspect paths on slow or disconnected - filesystems. The main loop could therefore freeze before `RemoveScreen`'s - existing background evaluator ever appeared. - -The Installed tab now owns an asynchronous remove preflight. Target construction -and compact eligibility both run on the thread pool with cancellation checks -before and between grouped targets. The tab displays inline busy feedback, -rejects repeated remove shortcuts, and cancels the preflight when its/app -lifetime ends so a stale dialog cannot open after navigation or shutdown. The -UI callback receives a completed `RemoveDialogPlan`; a simple result opens the -compact confirmation, while a complex result enters `RemoveScreen` with the -already captured targets and primary evaluation. The wizard consumes that -result rather than repeating native inspection, but still performs its required -fresh validation immediately before deletion. - -The analogous-path audit found no other production UI caller of -`RemoveTargetResolver.Evaluate`. Cleanup validation remains lazily enumerated by -its owned removal worker, and CLI validation has no Terminal.Gui-thread concern. -Regression tests prove the preflight evaluator runs on a different thread, -pre-cancellation prevents evaluation, and repeated Installed-tab remove requests -cannot overlap. - -The broader ownership review found and closed one adjacent race before opening -the new PR. The ordinary awaitable UI dispatcher stops waiting when its token is -canceled, even if a dispatched callback has already begun. That is appropriate -for short cosmetic callbacks but would let remove-preflight ownership end while -the callback was still inside `IApplication.Run` for the compact modal or -wizard. Remove activation now uses a start-aware owned dispatcher: cancellation -atomically rejects callbacks that remain queued, while a callback that won the -start race is awaited through its return. Dedicated tests cover both sides of -that boundary, including a `LongRunning` callback so thread-pool scheduling -cannot masquerade as ownership behavior. - -Copilot's PR #18 review then identified one remaining shared-presentation race: -Installed inventory loading and remove preflight independently cleared the same -spinner and footer. Both operations could overlap through refresh/scope actions, -so the first completion could hide the other's still-active feedback. The tab -now tracks the two activity owners separately and derives the shared display -from both, prioritizing the remove message while remove input remains gated. -Regression tests cover each completion order. diff --git a/docs/runbooks/release-rollback.md b/docs/runbooks/release-rollback.md index 6b369a6..aaaf9fd 100644 --- a/docs/runbooks/release-rollback.md +++ b/docs/runbooks/release-rollback.md @@ -31,12 +31,6 @@ For a published release that must be withdrawn: gh release delete --repo harder/gh-skillview --cleanup-tag -y ``` -Example: - -```bash -gh release delete v0.2.3 --repo harder/gh-skillview --cleanup-tag -y -``` - After deletion: 1. Confirm the release is gone: diff --git a/docs/superpowers/specs/2026-05-20-skillview-workflow-first-ux-design.md b/docs/superpowers/specs/2026-05-20-skillview-workflow-first-ux-design.md deleted file mode 100644 index 1ba171e..0000000 --- a/docs/superpowers/specs/2026-05-20-skillview-workflow-first-ux-design.md +++ /dev/null @@ -1,211 +0,0 @@ -# SkillView workflow-first UX design - -Date: 2026-05-20 - -## Goal - -Redesign SkillView's TUI so it feels as clean and legible as `winget-gui-tui`, while staying optimized for AI skill management instead of package management. - -The result should: - -- keep the shell calm, consistent, and keyboard-first, -- make install location, agent membership, provenance, and health obvious, -- reduce modal churn by keeping more work in one persistent shell, and -- use current Terminal.Gui 2.2.x patterns instead of older framework workarounds where newer APIs already solve the problem. - -## Design inputs - -This design is based on three sources: - -1. SkillView's current shell and workflow split in `SkillViewApp`, `InstalledTabView`, `UpdatesTabView`, and `SkillDetailPaneView`. -2. `winget-gui-tui`'s proven shell patterns: pill tabs, stable list/detail split, custom status strip, strong focus styling, and a persistent detail workspace. -3. Current Terminal.Gui 2.2.x usage patterns already validated in both repos: `Application.Create().Init()`, `HasFocusChanged`, `CollectionNavigator = null`, paste events on text inputs, and custom views where the stock controls still do not provide the right UX primitive. - -## Non-goals - -- Do not turn SkillView into a visual clone of `winget-gui-tui`. -- Do not flatten SkillView's agent- and location-aware workflows into package-style one-row-per-agent views. -- Do not remove advanced install and maintenance scenarios such as shared installs, symlinked installs, package-backed installs, scan-root diagnostics, or capability-gated actions. - -## Top-level model - -Replace the current top-level shell split with three primary workspaces: - -1. **Discover** — search, inspect, and install skills from remote or local sources. -2. **Installed** — browse the current inventory of physical installs and manage them in place. -3. **Changes** — triage updates, cleanup work, and diagnostics in one maintenance-focused workspace. - -`Doctor` stops being a peer top-level tab. Its information moves into **Changes** and into inline health badges throughout the shell. This keeps the information architecture as compact as `winget-gui-tui` without hiding the extra complexity SkillView needs. - -## Shell layout - -The shell keeps the strongest structural cues from `winget-gui-tui`: - -- a pill-style tab strip at the top, -- a one-line context bar below it, -- a stable primary/detail split near 60/40, -- heavy borders on the focused pane and lighter borders on the unfocused pane, and -- one bottom status strip that shows active facets on the left and the most relevant shortcuts on the right. - -The visible shell wording should prefer **Locations** or **Install locations** over **roots**. Keep **roots** for doctor-grade diagnostics only. - -## Workspace responsibilities - -### Discover - -Discover is the browsing and install workspace. It keeps the left-side results list and right-side detail pane, but the facets are SkillView-specific: - -- agent, -- owner, -- provenance or source, -- hidden-directory allowance, -- install target or location, and -- health or compatibility cues when available. - -Search state stays warm. The user can preview a skill, install it, return to the list, and continue exploring without losing query context or selection. - -Install flows should move away from full-screen modal churn. Use compact confirmation or input surfaces only where the user must choose a version, path, or capability-gated flag. - -### Installed - -Installed becomes the inventory truth view. Rows represent the physical install identified by `InstalledSkill.ResolvedPath`, not a single agent projection. - -Each row should make these dimensions visible without opening a modal: - -- skill name, -- agent membership, -- location, -- provenance or package source, -- symlink state, -- pinned state, and -- health or validity. - -The right pane becomes the action anchor for the selected install. It should show metadata, rendered or raw preview, logs on demand, and context-sensitive actions such as reveal path, open upstream, remove or unlink, cleanup, and jump into related maintenance work. - -### Changes - -Changes is the maintenance workspace. It groups the tasks that answer the same question: what needs attention now? - -It contains three related queues: - -1. updates, -2. cleanup candidates, and -3. diagnostics or scan issues. - -This keeps the top-level shell small while still giving SkillView space to explain shared installs, symlink cleanup, unsupported flags, package-backed lockfile state, and scan-root problems. - -## Interaction model - -The redesign should follow these interaction rules: - -- Keep the shell keyboard-first, but make the current focus unmistakable. -- Preserve selection and cursor anchor after refresh or rescan. -- Disable `TableView` type-to-search where it conflicts with view-level shortcuts by setting `CollectionNavigator = null`. -- Use one-line facet chips and status summaries instead of burying state in prose labels. -- Keep the right pane persistent across mode switches where practical so actions stay visually anchored to the selected item. -- Reserve full modal workflows for confirmation and narrow inputs, not as the default working surface. - -## Component plan - -### Header and navigation - -Keep the custom tab bar approach. Terminal.Gui's stock tab model is still focus-driven, and SkillView needs logical tabs whose active state is independent of which child control currently has focus. - -### Context bar - -Add a one-line context bar under the header. It should surface the active agent, location, provenance, pin, and health facets, plus quick filter state. This is the main replacement for scattered labels and per-tab state text. - -### Primary tables - -Keep `TableView`, but make the UX more deliberate: - -- stable column widths, -- stronger selected-row styling, -- row badges or compact columns for agent membership and health, -- predictable shortcut routing, and -- refresh behavior that preserves position where possible. - -### Detail workspace - -Evolve `SkillDetailPaneView` into a true detail workspace with: - -- metadata chips at the top, -- rendered, raw, and log subviews, -- context-aware action chips, and -- clearer grouping between metadata, content preview, and operation feedback. - -### Status strip - -Adopt a custom status-strip model closer to `winget-gui-tui` than Terminal.Gui's stock `StatusBar`. The bottom row should be informational first: left-side facet badges, center status, right-side shortcut hints, with graceful truncation when space is tight. - -## State and data flow - -Keep one authoritative shell state with generation-guarded async updates. The top-level state should track: - -- active workspace, -- active facet set, -- current selection, -- current detail payload, -- latest inventory snapshot, and -- current maintenance queue state. - -Each workspace can keep small local state, such as Discover's query text or Changes' active maintenance section, but inventory and selection truth should stay centralized. - -Operationally, actions should follow one loop: - -1. capture or refresh inventory, -2. derive visible rows from the snapshot and active facets, -3. load richer detail for the selected item, -4. execute the selected action, and -5. rescan and restore context in the same shell. - -## Error handling and feedback - -Errors should stop feeling like detached log blobs. - -Capability gaps, invalid installs, broken symlinks, unsupported flags, and path-safety failures should surface in three layers: - -1. inline health badges on the affected row, -2. right-pane explanation and next-step text for the selected item, and -3. status-strip summary for the current operation. - -Logs remain available, but as a drill-down view, not the primary place the user learns that something went wrong. - -## Terminal.Gui guidance - -Use the current Terminal.Gui 2.2.x path already validated in this repo and in `winget-gui-tui`: - -- keep the instance-based `Application.Create().Init()` lifecycle, -- use `HasFocusChanged` to drive focused-pane emphasis, -- use text-input paste events where they improve search and filter ergonomics, -- keep custom views for the tab strip, status strip, and rich detail presentation where Terminal.Gui still lacks the right primitive, and -- avoid reviving workarounds for older `TableView` behavior that 2.2.x already replaced. - -## Testing - -Treat the shell behavior as a UX contract and test it accordingly. - -Add or expand coverage around: - -- top-level workspace switching, -- context-bar and status-strip content, -- focused vs unfocused border styling, -- cursor restoration after refresh, -- action availability under capability gates, -- health badge and detail rendering for invalid or shared installs, -- changes-queue behavior for updates, cleanup, and diagnostics, and -- the physical-install inventory model where one install can belong to multiple agents. - -For the custom Terminal.Gui views, keep small targeted tests that protect the specific surfaces this redesign depends on, the same way `winget-gui-tui` guards its custom shell widgets against Terminal.Gui upgrades. - -## Recommended rollout - -Implement the redesign in this order: - -1. shell chrome and shared component upgrades, -2. Discover workspace reshape, -3. Installed workspace reshape, -4. Changes workspace introduction and Doctor absorption, and -5. focused tests for the new UX contract. - -This keeps the app usable throughout the redesign and lets the new shell land before the deeper workflow migration. diff --git a/docs/usage.md b/docs/usage.md index 91ce555..0cc3669 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -13,7 +13,7 @@ The TUI is organized around three primary tabs in a persistent top header — ** | **Discover** ◇ | Search public skills, refine by owner/agent/limit, preview `SKILL.md`, inspect metadata, flip the right pane between preview and logs, and stage installs. Default landing view. | `1`, click the pill, or `←/→` to cycle | | **Installed** ▣ | Lists installed skills across the local inventory, filter, cycle sort, cycle a pinned/unpinned filter, inspect details, open the folder, or remove. | `2`, click the pill, or `←/→` | | **Changes** △ | Maintenance queue showing pending cleanup tasks. Press Enter to open the appropriate specialist view. | `3`, `u`, click the pill, or `←/→` | -| **Doctor** | Full-screen environment report: `gh` path/version, auth state, detected capabilities, installed agent homes, and log location. Esc returns to the previous tab. | `d` | +| **Doctor** | Full-screen environment report: `gh` path/version, auth state, `gh skill` availability, installed agent homes, and log location. Esc returns to the previous tab. | `d` | | **Install — compact** | One-screen confirm: scope radio, agent checkboxes pre-selected from your home directory, **Install** / **Advanced…** / **Cancel**. | `i` from a Discover result | | **Install — advanced wizard** | Full multi-step dialog with version, scope, agent, path, overwrite, and options for hidden-dir scanning, installing from a republished skill's upstream source, or local installs. | `I` from a Discover result, or **Advanced…** from the compact modal | | **Remove — compact** | `[y]es / [n]o` confirm for simple single-skill removes. | `x` from an Installed row whose plan is straightforward | @@ -103,7 +103,7 @@ SkillView runs in CLI mode when you provide a subcommand. | Command | What it is for | |---|---| -| `skillview doctor` | Inspect environment, auth, capabilities, and log paths | +| `skillview doctor` | Inspect environment, auth, `gh skill` availability, and log paths | | `skillview list` | Show installed skills from filesystem and, when available, `gh skill list` | | `skillview rescan` | Re-run inventory capture and print a summary | | `skillview search ` | Search public repositories for skills | @@ -124,6 +124,15 @@ skillview update --dry-run skillview cleanup --apply --yes ``` +For `install`, use `--agent ` to choose an agent and `--scope project|user|custom` +to choose the destination scope. `--path ` selects a custom directory; +`--all` installs every skill found in a repository. To install a specific ref, +pass `--version ` or use `OWNER/REPO@` as shorthand. Add `--pin` only +when a ref is supplied; for an entire repository, `OWNER/REPO@ --all --pin` +pins that ref. `--upstream` is a switch for a republished skill's declared +upstream source, not a URL argument. Run `skillview --help` for the +complete option list. + ### Global flags ```bash @@ -162,7 +171,7 @@ SkillView's CLI is designed to be automation-friendly when you want higher-level - Prefer `--json` on commands that support it: `doctor`, `list`, `search`, `preview`, `install`, `update`, `remove`, and `cleanup`. - Use exit codes as the control surface for scripts: `0` success, `2` invalid usage, `10` environment/setup problems, `20` no matches, `130` canceled. - Put global flags like `--scan-root` and `--theme` **before** the subcommand; only `--debug` is accepted after the subcommand. -- `skillview doctor --json` is the fastest way for an agent or script to confirm `gh` version, auth state, capability probes, and log location before attempting an install/update flow. +- `skillview doctor --json` reports the `gh` version, auth state, `gh skill` availability, and log location before an install or update flow. Examples: diff --git a/site/docs.html b/site/docs.html index 5b9b562..0285420 100644 --- a/site/docs.html +++ b/site/docs.html @@ -36,7 +36,7 @@ skillview update --dry-run skillview cleanup

Commands that support --json can feed scripts and AI agents. Run skillview --help for flags and options. Put global flags such as --scan-root before the subcommand:

skillview --scan-root /path/to/skills list --json

When something feels off

Run skillview doctor to inspect your GitHub CLI path, version, authentication, detected agent homes, and log location. In the TUI, press d.

For a bug report, include skillview --version, your OS and terminal, the command or flow you used, and whether gh auth status succeeds. Run with --debug if you need a more detailed log.

-

Go deeper

The repository’s complete usage guide covers every shortcut, CLI flags, exit codes, configuration, and troubleshooting. For development setup and tests, see CONTRIBUTING.md.

SkillView is open source under the MIT License. Browse the source on GitHub or visit Kevin Harder and Harder Labs.

+

Go deeper

The repository’s usage guide covers shortcuts, common CLI flags, exit codes, configuration, and troubleshooting. Run skillview --help for the complete command option list. For development setup and tests, see CONTRIBUTING.md.

SkillView is open source under the MIT License. Browse the source on GitHub or visit Kevin Harder and Harder Labs.

diff --git a/src/SkillView.Core/Cli/CliDispatcher.cs b/src/SkillView.Core/Cli/CliDispatcher.cs index 8ae541e..0d01df7 100644 --- a/src/SkillView.Core/Cli/CliDispatcher.cs +++ b/src/SkillView.Core/Cli/CliDispatcher.cs @@ -120,7 +120,7 @@ internal static string RenderDoctorText(EnvironmentReport r, AppOptions options) sb.AppendLine($"gh version : {r.GhVersionRaw ?? "(unknown)"}"); sb.AppendLine($"gh minimum : {GhBinaryLocator.MinimumVersion}{(r.GhMeetsMinimum ? " ✓" : " ✗ too old")}"); sb.AppendLine($"gh auth : {AuthSummary(r.Auth)}"); - sb.AppendLine($"gh skill : {(r.GhSkillAvailable ? "present (gh ≥ 2.95 — full skill surface)" : "(not detected)")}"); + sb.AppendLine($"gh skill : {(r.GhSkillAvailable ? "present" : "(not detected)")}"); sb.AppendLine($"debug : {options.Debug}"); sb.AppendLine($"log directory : {r.LogDirectory ?? "(unset)"}"); sb.AppendLine($"scan roots : {(options.ScanRoots.Count == 0 ? "(default)" : string.Join(", ", options.ScanRoots))}"); @@ -168,9 +168,8 @@ private static void WriteDoctorJson(Utf8JsonWriter writer, EnvironmentReport r, writer.WriteEndArray(); writer.WriteEndObject(); - // gh ≥ 2.95 is required, so the full `gh skill` flag surface is - // guaranteed once the command is present — a single availability - // bool replaces the old per-flag capability probe. + // The minimum supported gh version guarantees the required command + // surface; the smoke check records whether `gh skill` is available. writer.WriteBoolean("ghSkillAvailable", r.GhSkillAvailable); writer.WriteStartArray("scanRoots"); @@ -1692,7 +1691,7 @@ You are running the `{{command}}` entrypoint. The alternate entrypoint is `{{alt | Subcommand | Purpose | Key options | | --- | --- | --- | - | `doctor` | Inspect `gh`, auth state, capability probes, log path, and scan roots. | `--json`, `--clear-logs` | + | `doctor` | Inspect `gh`, auth state, `gh skill` availability, log path, and scan roots. | `--json`, `--clear-logs` | | `list` | Show installed skills from the filesystem and, when supported, `gh skill list`. | `--json`, `--scope`, `--agent`, `--dir`, `--allow-hidden-dirs` | | `rescan` | Rebuild the local inventory snapshot and print a summary. | _none_ | | `search ` | Search public skill repositories. | `--owner`, `--limit`, `--page`, `--json` | @@ -1725,7 +1724,7 @@ SkillView is automation-friendly when you want safer wrappers than raw `gh skill ## Notes - - SkillView only emits capability-gated flags when the installed `gh` build supports them. + - SkillView requires `gh` 2.97.0 or newer and checks that `gh skill` is available. - `--debug` is the only global flag accepted after a subcommand. Put other global flags before the subcommand. - `SKILLVIEW_LOG=debug` is the environment-variable alternative to `--debug`. - Homebrew and WinGet scaffolding exists in the release workflow, but those channels are still dark-launch only and are not public install paths yet. diff --git a/tests/SkillView.Tests/Cli/CliDispatcherHelpTests.cs b/tests/SkillView.Tests/Cli/CliDispatcherHelpTests.cs index 26c2614..d08510e 100644 --- a/tests/SkillView.Tests/Cli/CliDispatcherHelpTests.cs +++ b/tests/SkillView.Tests/Cli/CliDispatcherHelpTests.cs @@ -24,6 +24,8 @@ public async Task HelpFlag_PrintsMarkdownHelp() Assert.DoesNotContain("`--json`, `--scope`, `--agent`, `--path`, `--allow-hidden-dirs`", stdout); Assert.Contains("Homebrew and WinGet scaffolding", stdout); Assert.Contains("automation-friendly", stdout); + Assert.Contains("SkillView requires `gh` 2.97.0 or newer", stdout); + Assert.DoesNotContain("capability-gated flags", stdout); Assert.Contains("| `130` | Canceled by the caller or Ctrl+C |", stdout); } diff --git a/tests/SkillView.Tests/Subprocess/ProcessRunnerTests.cs b/tests/SkillView.Tests/Subprocess/ProcessRunnerTests.cs index ab64146..9407978 100644 --- a/tests/SkillView.Tests/Subprocess/ProcessRunnerTests.cs +++ b/tests/SkillView.Tests/Subprocess/ProcessRunnerTests.cs @@ -85,7 +85,10 @@ public async Task RunAsync_CancellationTerminatesWithinBoundedWait() await Assert.ThrowsAnyAsync(() => runner.RunAsync(executable, arguments, cancellationToken: cancellation.Token)); - Assert.InRange(stopwatch.Elapsed, TimeSpan.Zero, TimeSpan.FromSeconds(4)); + // Windows CI can spend several seconds starting and terminating pwsh + // under load. Keep this well below the child's 30-second natural exit + // without treating scheduler delay as a subprocess cancellation bug. + Assert.InRange(stopwatch.Elapsed, TimeSpan.Zero, TimeSpan.FromSeconds(10)); } private static (string Executable, string[] Arguments) CreateWaitForEofCommand()