fix(setup): stop enable --agent from clobbering unparseable settings

ab01d5f→main·

suhaanthayyil·3d ago·2 files·+63 added/-13 removed

setupAgentHooksNonInteractive loaded the target file via settings.LoadFromFile and, on any error, replaced it with a defaults struct that saveEnabledState then wrote back. Because LoadFromFile fails on invalid JSON AND on any unknown key (DisallowUnknownFields), a settings.json with strategy_options/log_level or a key from a newer CLI was silently destroyed and rewritten as {"enabled": true}.

Refuse with a clear error naming the file instead of clobbering it (a missing file still returns defaults, so first-time enable is unaffected), mirroring updateStrategyOptions. Also warn instead of silently degrading to the scoped view when the merged-settings load for hook installation fails. Rename the local settings var to targetSettings so it no longer shadows the settings package import.

Changes

2

• cmd/entire/cli

• Msetup.go+22/-13

• Msetup_test.go+41

1808 unmodified lines

1809
1810
1811
1812
1813
1812
1813
1814
1815
1816
1817
1818
1819
1820
1821
1822
1823
1815
1816
1824
1825
1818
1826
1827
1820
1828
1829
1830
1823
1831
1832
1833
1834
1835
1828
1836
1837
1838
1831
1839
1840
1841
1834
1842
1843
1844
1845
1 unmodified line

1847
1848
1849
1842
1850
1851
1852
1845
1853
1854
1855
1856
8 unmodified lines

1865
1866
1867
1860
1868
1869
1870
1871
1872

1808 unmodified lines

// Load existing settings from the target file only, to preserve other
    // options already set there (like strategy_options.push) without pulling
    // in the other scope's overrides.
    settings, err := settings.LoadFromFile(targetFileAbs)
    // in the other scope's overrides. The local var is named targetSettings so
    // it does not shadow the settings package for the rest of the function.
    //
    // On a parse/validation failure we refuse rather than start from defaults:
    // the previous behavior silently replaced a settings.json holding real
    // content (strategy_options, log_level, and — under DisallowUnknownFields —
    // any key written by a newer CLI) with a bare {"enabled": true}, destroying
    // the user's config. A missing file is NOT an error here (LoadFromFile
    // returns defaults for it), so first-time enable still works. This mirrors
    // updateStrategyOptions, which already refuses on an unparseable target file.
    targetSettings, err := settings.LoadFromFile(targetFileAbs)
    if err != nil {
        // If we can't load, start with defaults
        settings = &EntireSettings{}
        return fmt.Errorf("refusing to enable: %s could not be parsed (invalid JSON, or written by a newer entire version); fix or remove it, or upgrade the CLI, then retry: %w", configDisplay, err)
    }
    settings.Enabled = true
    targetSettings.Enabled = true
    if opts.LocalDev {
        settings.LocalDev = true
        targetSettings.LocalDev = true
    }
    if opts.AbsoluteGitHookPath {
        settings.AbsoluteGitHookPath = true
        targetSettings.AbsoluteGitHookPath = true
    }

// Auto-enable external_agents setting if the agent is external.
    if external.IsExternal(ag) {
        settings.ExternalAgents = true
        targetSettings.ExternalAgents = true
    }

opts.applyStrategyOptions(settings)
    opts.applyStrategyOptions(targetSettings)

// Apply an explicit --checkpoint-backend (no prompt on this non-interactive path).
    if err := applyCheckpointBackendFlag(settings, opts.CheckpointBackend); err != nil {
    if err := applyCheckpointBackendFlag(targetSettings, opts.CheckpointBackend); err != nil {
        return err
    }

1 unmodified line

// Note: if telemetry is nil (not configured), it defaults to disabled
    if !opts.Telemetry || os.Getenv("ENTIRE_TELEMETRY_OPTOUT") != "" {
        f := false
        settings.Telemetry = &f
        targetSettings.Telemetry = &f
    }

if err := saveEnabledState(ctx, settings, targetFile == EntireSettingsFile); err != nil {
    if err := saveEnabledState(ctx, targetSettings, targetFile == EntireSettingsFile); err != nil {
        return fmt.Errorf("failed to save settings: %w", err)
    }

8 unmodified lines

// (see the comment on saveEnabledState for why).
    mergedSettings, err := LoadEntireSettings(ctx)
    if err != nil {
        mergedSettings = settings
        logging.Warn(ctx, "could not load merged settings for hook installation; proceeding with target-scoped settings only, so local overrides (e.g. local_dev, absolute_git_hook_path) may not be applied to the generated git hook", "error", err)
        mergedSettings = targetSettings
    }
    hookLocalDev := mergedSettings.LocalDev || opts.LocalDev
    hookAbsoluteGitHookPath := mergedSettings.AbsoluteGitHookPath || opts.AbsoluteGitHookPath

Mcmd/entire/cli/setup.go+22/-13

544 unmodified lines

545
546
547
548
549
550
551
552
553
554
555
556
557
558
559
560
561
562
563
564
565
566
567
568
569
570
571
572
573
574
575
576
577
578
579
580
581
582
583
584
585
586
587
588
589
590
591

544 unmodified lines

}

// TestSetupAgentHooksNonInteractive_RefusesToClobberUnparseableSettings covers
// the finding that `entire enable --agent` silently wiped a corrupt or
// newer-versioned target settings file to defaults. settings.LoadFromFile
// errors on invalid JSON AND on any unknown key (DisallowUnknownFields); the
// old catch replaced the struct with defaults and wrote it back, so a
// settings.json with strategy_options/log_level/one-unknown-key became exactly
// {"enabled": true}. Now it refuses and leaves the file untouched.
func TestSetupAgentHooksNonInteractive_RefusesToClobberUnparseableSettings(t *testing.T) {
    setupTestRepo(t)
    // A settings.json a newer CLI could write: valid JSON, real content, plus a
    // key this build doesn't recognize (rejected by DisallowUnknownFields).
    original := `{\"enabled\": false, \"log_level\": \"debug\", \"totally_unknown_future_key\": 42}`
    writeSettings(t, original)
    writeClaudeHooksFixture(t)

ag, err := agent.Get(types.AgentName("claude-code"))
    if err != nil {
        t.Fatalf("agent.Get(claude-code) error = %%v", err)
    }

var buf bytes.Buffer
    if err := setupAgentHooksNonInteractive(context.Background(), &buf, ag, EnableOptions{}); err == nil {
        t.Fatal("expected setupAgentHooksNonInteractive to refuse on an unparseable settings file, got nil error")
    }

// The file must be left as-is, not wiped to {"enabled": true}.
    got, err := os.ReadFile(EntireSettingsFile)
    if err != nil {
        t.Fatalf("failed to read project settings: %%v", err)
    }
    if !strings.Contains(string(got), "totally_unknown_future_key") {
        t.Errorf("unknown key must survive (file must not be clobbered), got: %%s", got)
    }
    if !strings.Contains(string(got), "log_level") {
        t.Errorf("log_level must survive (file must not be clobbered), got: %%s", got)
    }
    if strings.Contains(string(got), "\"enabled\": true") || strings.Contains(string(got), "\"enabled\":true") {
        t.Errorf("enabled must not have been flipped/rewritten, got: %%s", got)
    }
}

func TestRunDisable(t *testing.T) {
    setupTestDir(t)
    writeSettings(t, testSettingsEnabled)