From 6167ffa18614b75712e652876899221087b0d275 Mon Sep 17 00:00:00 2001 From: Alexgodoroja Date: Fri, 17 Jul 2026 12:57:00 -0700 Subject: [PATCH] tick: make the disabled opt-out survive ForceTick MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `pilotctl skills disable` set mode=disabled, but ForceTick bypassed the gate entirely (it took a `force` flag whose only effect was to skip the disabled check). ForceTick backs `pilotctl skills check`, `pilotctl update`'s post-update reconcile, and the installer's first-pass — so any of those re-injected skills a user had explicitly turned off. The opt-out did not survive an update. Gate on !dryRun instead of !force: disabled is now a hard opt-out for every write path, while the read-only Plan() (behind `pilotctl skills` status) still previews. The vestigial `force` parameter is removed; ForceTick stays as the immediate-reconcile entry point (enable persists mode=auto before calling in, so re-enable still works). Rewrites the test that pinned the old bypass and adds one for Plan previewing while disabled. --- skillinject.go | 40 +++++++++++++++++++------------- zz_extra_branches_test.go | 49 +++++++++++++++++++++++++++++---------- 2 files changed, 61 insertions(+), 28 deletions(-) diff --git a/skillinject.go b/skillinject.go index 84122e8..71e86d1 100644 --- a/skillinject.go +++ b/skillinject.go @@ -128,25 +128,28 @@ func Run(ctx context.Context, cfg Config) { // If the user has set skill injection to disabled mode via // `pilotctl skills disable` (persisted in ~/.pilot/config.json), // Tick returns an empty report without touching disk or the network. -// Use ForceTick to run even when skill injection is disabled (e.g. -// post-update reconcile in manual mode). func Tick(ctx context.Context, cfg Config) (*Report, error) { tickMu.Lock() defer tickMu.Unlock() - return tick(ctx, cfg, false, false) + return tick(ctx, cfg, false) } -// ForceTick is like Tick but skips the IsEnabled() gate. It performs a -// full scan + reconcile pass regardless of the persisted skill_inject -// flag. Intended for one-shot use after a manual update (pilotctl update) -// so skills reconcile even in manual mode where the periodic ticker is -// not running. +// ForceTick performs an immediate scan + reconcile pass, bypassing the +// periodic ticker (which does not run in manual mode). It is the entry +// point for `pilotctl skills check`, `pilotctl skills enable`, and the +// post-update reconcile in `pilotctl update`. +// +// It does NOT override the disabled opt-out: once the user has run +// `pilotctl skills disable`, ForceTick is a no-op until they re-enable. +// (Enable persists mode=auto before calling in, so its reconcile still +// runs.) Manual mode is not gated here, so a forced reconcile there +// behaves exactly like Tick. func ForceTick(ctx context.Context, cfg Config) (*Report, error) { tickMu.Lock() defer tickMu.Unlock() - return tick(ctx, cfg, true, false) + return tick(ctx, cfg, false) } // Plan is a read-only dry run: it performs the same scan + classification @@ -158,15 +161,15 @@ func Plan(ctx context.Context, cfg Config) (*Report, error) { tickMu.Lock() defer tickMu.Unlock() - return tick(ctx, cfg, true, true) + return tick(ctx, cfg, true) } -// tick is the shared implementation for Tick and ForceTick. When force is -// true, the IsEnabled gate is bypassed. +// tick is the shared implementation for Tick, ForceTick, and Plan. dryRun +// classifies without writing (Plan); otherwise it reconciles to disk. // -// Callers (Tick, ForceTick) hold tickMu before calling tick; do NOT -// acquire it here or we deadlock. -func tick(ctx context.Context, cfg Config, force, dryRun bool) (*Report, error) { +// Callers hold tickMu before calling tick; do NOT acquire it here or we +// deadlock. +func tick(ctx context.Context, cfg Config, dryRun bool) (*Report, error) { home := cfg.Home if home == "" { h, err := os.UserHomeDir() @@ -176,7 +179,12 @@ func tick(ctx context.Context, cfg Config, force, dryRun bool) (*Report, error) home = h } - if !force && GetMode(home) == ModeDisabled { + // Disabled is a hard opt-out: once the user runs `pilotctl skills disable`, + // nothing WRITES skills back — not the periodic ticker, and not a forced + // reconcile from `pilotctl skills check`, `pilotctl update`, or an installer + // re-run. Only the read-only dry run (Plan, behind `pilotctl skills` status) + // still previews what a re-enable would do. + if !dryRun && GetMode(home) == ModeDisabled { return &Report{At: time.Now().UTC(), Disabled: true}, nil } diff --git a/zz_extra_branches_test.go b/zz_extra_branches_test.go index 1b766fa..eec7a1e 100644 --- a/zz_extra_branches_test.go +++ b/zz_extra_branches_test.go @@ -192,29 +192,54 @@ func TestTick_DisabledReturnsEarly(t *testing.T) { } } -// skillTargetPath: "flat" SkillNaming yields a single-file path. -func TestForceTick_SkipsDisabledGate(t *testing.T) { +// ForceTick must HONOR the disabled opt-out — it does not resurrect skills a +// user turned off via `pilotctl skills disable`. This is what protects the +// opt-out from `pilotctl skills check`, `pilotctl update`, and installer +// re-runs (all of which go through ForceTick). +func TestForceTick_HonorsDisabledGate(t *testing.T) { t.Parallel() home := t.TempDir() if err := SetEnabled(home, false); err != nil { t.Fatalf("SetEnabled(false): %v", err) } - // Use a bogus URL — if ForceTick actually reaches the network, it - // would error. We give it a manifest URL that errors so we can tell - // ForceTick ran past the IsEnabled gate (returns network error) - // instead of the disabled short-circuit (returns Disabled report). + // Bogus URL: if ForceTick reached the network it would error. A disabled + // config must short-circuit BEFORE any fetch — so we expect no error and a + // Disabled report, proving nothing was fetched or written. cfg := Config{ Home: home, ManifestURL: "http://127.0.0.1:1/nonexistent.json", RepoBaseURL: "http://127.0.0.1:1/", } - _, err := ForceTick(context.Background(), cfg) - if err == nil { - t.Error("ForceTick on disabled config with bogus URL: expected network error, got nil") + rep, err := ForceTick(context.Background(), cfg) + if err != nil { + t.Fatalf("ForceTick on disabled config: expected no error (short-circuit), got %v", err) + } + if rep == nil || !rep.Disabled { + t.Errorf("ForceTick on disabled config: expected Disabled report, got %+v", rep) + } + if rep != nil && (len(rep.Outcomes) != 0 || len(rep.Skipped) != 0) { + t.Errorf("ForceTick on disabled config wrote/planned work: %+v", rep) + } +} + +// Plan (the read-only dry run behind `pilotctl skills` status) still previews +// even when disabled — it must reach the network to classify, so a bogus URL +// makes it error rather than short-circuit. This pins that the opt-out gate is +// write-only, not a status blackout. +func TestPlan_PreviewsEvenWhenDisabled(t *testing.T) { + t.Parallel() + home := t.TempDir() + if err := SetEnabled(home, false); err != nil { + t.Fatalf("SetEnabled(false): %v", err) + } + cfg := Config{ + Home: home, + ManifestURL: "http://127.0.0.1:1/nonexistent.json", + RepoBaseURL: "http://127.0.0.1:1/", + } + if _, err := Plan(context.Background(), cfg); err == nil { + t.Error("Plan on disabled config with bogus URL: expected a network error (it must still fetch to preview), got nil") } - // The error should be a network error, not a disabled report. - // The disabled report would have succeeded with no error. - t.Logf("ForceTick error (expected network error): %v", err) } func TestSkillTargetPath_FlatNaming(t *testing.T) {