Skip to content

feat: migrate config format to TOML and add CLI management - #515

Merged
SuperCoolPencil merged 22 commits into
mainfrom
not-json
Jun 24, 2026
Merged

feat: migrate config format to TOML and add CLI management#515
SuperCoolPencil merged 22 commits into
mainfrom
not-json

Conversation

@SuperCoolPencil

@SuperCoolPencil SuperCoolPencil commented Jun 24, 2026

Copy link
Copy Markdown
Member

Fixes #509

This PR migrates Surge's configuration files from JSON to TOML and introduces a headless surge config CLI utility.

Changes:

  • TOML Migration: Changed the default config file from settings.json to settings.toml. Integrated go-toml/v2 to robustly handle parsing and mapping configurations dynamically without breaking backward compatibility for the older file formats (like keymap.json).
  • Headless Configuration: Implemented cmd/config.go providing surge config [Category].[Key] [Value] capability.
  • Improved Help: Running surge config lists all categories, keys, types, values, and human-readable descriptions.
  • Config Validation Fixes: Refactored the ValidateFunc logic in internal/config/settings.go to properly support int64 assertions introduced by the new TOML parser.
  • Docs Update: Updated SETTINGS.md and USAGE.md to reflect the new TOML formatting and CLI commands.

Greptile Summary

This PR migrates Surge's configuration from JSON to TOML using pelletier/go-toml/v2 (already a transitive dependency) and adds a headless surge config / surge category CLI for viewing and modifying settings without opening the TUI.

  • TOML migration: LoadSettings parses TOML into a map[string]any, assigns values to typed Setting pointers, then normalizes them via Resolve() before running Validate(). Atomic writes via temp-file-then-rename preserve data integrity. Legacy settings.json users are warned on first launch.
  • Headless CLI: configCmd supports list/search/get/set/reset/open; categoryCmd adds list/add/remove subcommands. Type-safe SetSetting covers bool, int, int64, float64, duration, and string types, with accompanying tests.
  • Test coverage: New cli_test.go covers ParseConfigPath, GetSetting, SetSetting, and ResetSetting; config_test.go covers integration flows end-to-end; a regression suite guards against silent startup-warning suppression.

Confidence Score: 4/5

Safe to merge with minor cleanup; the TOML migration is well-structured and the new CLI is adequately tested.

The core migration logic is sound — TOML round-trips are correct, type normalization via Resolve() handles all coercions, and atomic writes protect against partial writes. Open items from prior review rounds (EDITOR arg-splitting, float64 validator int64 gap) remain unaddressed. New findings are limited to redundant URL validation, unused resources from PersistentPreRun on config subcommands, a missing description parameter in category add, and a potentially flaky test in CI — none affecting production correctness.

internal/config/settings.go (float64 validators missing int64 case from prior review), cmd/config.go (EDITOR arg-splitting still unaddressed), cmd/root.go (pre-validation duplication and PersistentPreRun resource creation for subcommands)

Important Files Changed

Filename Overview
internal/config/settings.go Core settings migration from JSON to TOML: adds LoadSettings/SaveSettings TOML round-trip, Resolve() type normalization, and startup warning for legacy settings.json. Float64 validator int64 gap flagged in prior review.
internal/config/cli.go New ParseConfigPath/GetSetting/SetSetting/ResetSetting functions for headless config management; type-safe parsing for all setting types. Tests added in cli_test.go.
cmd/config.go New headless config CLI: list/search/get/set/reset/open subcommands. EDITOR arg-splitting issue flagged in prior review; pre-validation double-parse noted here.
cmd/config_category.go New category management CLI (list/add/remove/show). The add subcommand omits the Description field with no way to set it via CLI.
cmd/root.go Adds pre-flight URL validation before acquiring instance lock; validation logic is duplicated with processDownloads and PersistentPreRun creates unused resources for config/category subcommands.
internal/config/config_warning_regression_test.go Comprehensive regression suite for startup warnings on corrupt/truncated/missing/valid TOML files. ValidSettings test may be flaky in CI if the default downloads directory doesn't exist.
cmd/config_test.go New integration tests for the config CLI command covering list, get, search, set/reset, and the open-editor path with a dummy editor script.
go.mod Promotes pelletier/go-toml/v2 from indirect to direct dependency; already used by colors.go so no new library is introduced.

Comments Outside Diff (3)

  1. go.mod, line 279 (link)

    P1 Second TOML library added when one already exists

    BurntSushi/toml is already a direct dependency (used in internal/tui/colors/colors.go for theme files) and supports map[string]any unmarshaling. Adding pelletier/go-toml/v2 for settings parsing means the binary now ships two TOML parsers. Both libraries overlap entirely in purpose. Either use BurntSushi/toml for settings as well, or migrate the theme parser to go-toml/v2 — keeping both violates the project's lightweight-dependency policy.

    Rule Used: What: Reject unnecessary dependencies, abstraction... (source)

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: go.mod
    Line: 279
    
    Comment:
    **Second TOML library added when one already exists**
    
    `BurntSushi/toml` is already a direct dependency (used in `internal/tui/colors/colors.go` for theme files) and supports `map[string]any` unmarshaling. Adding `pelletier/go-toml/v2` for settings parsing means the binary now ships two TOML parsers. Both libraries overlap entirely in purpose. Either use `BurntSushi/toml` for settings as well, or migrate the theme parser to `go-toml/v2` — keeping both violates the project's lightweight-dependency policy.
    
    **Rule Used:** What: Reject unnecessary dependencies, abstraction... ([source](https://app.greptile.com/surge-org-3/github/surge-downloader/surge/-/custom-context?memory=56e90a47-406e-439b-895b-903cf8c985f1))
    
    How can I resolve this? If you propose a fix, please make it concise.

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. internal/config/settings.go, line 1201-1215 (link)

    P2 Clone() uses JSON round-trip, changing value types after TOML load

    After loading from TOML, integer settings are stored as int64 in Setting.Value. json.Marshal serializes them as JSON numbers; json.Unmarshal via Setting.UnmarshalJSON then stores them as float64. The Resolve[T] helpers handle this, so callers using Resolve are safe. However, any direct type assertion on Value (e.g., val.(int64)) would panic on a cloned object even if the original loaded correctly. Consider implementing Clone in terms of SaveSettings/LoadSettings with a temp buffer, or via an explicit field-by-field copy, to keep the type contract consistent after the TOML migration.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: internal/config/settings.go
    Line: 1201-1215
    
    Comment:
    **`Clone()` uses JSON round-trip, changing value types after TOML load**
    
    After loading from TOML, integer settings are stored as `int64` in `Setting.Value`. `json.Marshal` serializes them as JSON numbers; `json.Unmarshal` via `Setting.UnmarshalJSON` then stores them as `float64`. The `Resolve[T]` helpers handle this, so callers using `Resolve` are safe. However, any direct type assertion on `Value` (e.g., `val.(int64)`) would panic on a cloned object even if the original loaded correctly. Consider implementing `Clone` in terms of `SaveSettings`/`LoadSettings` with a temp buffer, or via an explicit field-by-field copy, to keep the type contract consistent after the TOML migration.
    
    How can I resolve this? If you propose a fix, please make it concise.
  3. internal/config/settings.go, line 890-904 (link)

    P1 float64 validators miss the int64 case added by this PR's TOML migration. go-toml/v2 unmarshals TOML integers (e.g., slow_worker_threshold = 0) as int64, not float64. The validators for SlowWorkerThreshold and SpeedEmaAlpha only handle float64 and int, so a TOML-integer value hits the default branch and returns "invalid type" — silently resetting those settings to defaults. The int validators in this same diff were fixed (adding int64 handling), but the float64 validators were not. Add case int64: v = float64(actual) to both validators.

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: internal/config/settings.go
    Line: 890-904
    
    Comment:
    `float64` validators miss the `int64` case added by this PR's TOML migration. `go-toml/v2` unmarshals TOML integers (e.g., `slow_worker_threshold = 0`) as `int64`, not `float64`. The validators for `SlowWorkerThreshold` and `SpeedEmaAlpha` only handle `float64` and `int`, so a TOML-integer value hits the `default` branch and returns "invalid type" — silently resetting those settings to defaults. The `int` validators in this same diff were fixed (adding `int64` handling), but the `float64` validators were not. Add `case int64: v = float64(actual)` to both validators.
    
    How can I resolve this? If you propose a fix, please make it concise.

Reviews (12): Last reviewed commit: "feat: implement shell completion for con..." | Re-trigger Greptile

@SuperCoolPencil SuperCoolPencil changed the title feat: migrate config format to TOML and add CLI management (#509) feat: migrate config format to TOML and add CLI managemen5 Jun 24, 2026
@SuperCoolPencil SuperCoolPencil changed the title feat: migrate config format to TOML and add CLI managemen5 feat: migrate config format to TOML and add CLI management Jun 24, 2026
@SuperCoolPencil SuperCoolPencil linked an issue Jun 24, 2026 that may be closed by this pull request
@github-actions

github-actions Bot commented Jun 24, 2026

Copy link
Copy Markdown

Binary Size Analysis

⚠️ Size Increased

Version Human Readable Raw Bytes
Main 16.70 MB 17514788
PR 16.98 MB 17809700
Difference 288.00 KB 294912

Comment thread internal/config/cli.go Fixed
Comment thread internal/config/settings.go
Comment thread cmd/config.go
Comment thread internal/config/cli.go
Comment thread internal/config/config_warning_regression_test.go Outdated
@SuperCoolPencil
SuperCoolPencil force-pushed the not-json branch 2 times, most recently from dc010fd to 6cf6030 Compare June 24, 2026 05:30
Comment thread internal/config/settings.go
Comment thread cmd/root.go
@SuperCoolPencil

SuperCoolPencil commented Jun 24, 2026

Copy link
Copy Markdown
Member Author

@greptileai

All three comments are addressed:

  1. "Second TOML library" — Incorrect. Only pelletier/go-toml/v2 exists in go.mod. colors.go already uses the same library. No BurntSushi/toml present.
  2. "Clone() uses JSON round-trip" — Already fixed. Clone() at settings.go:1248 uses direct field-by-field copy via FindSettingsCategory/FindSetting — no JSON marshal involved.
  3. "float64 validators miss int64 case" — Already present. Both slow_worker_threshold (settings.go:876) and speed_ema_alpha (settings.go:951) include case int64: v = float64(actual).

@SuperCoolPencil
SuperCoolPencil merged commit b6329cf into main Jun 24, 2026
14 checks passed
@SuperCoolPencil
SuperCoolPencil deleted the not-json branch June 24, 2026 12:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Proposal] Migrate configuration to use toml instead of json Feature: CLI ability to manage settings (surge config)

2 participants