crushrc: bash-based config - #3410
Conversation
joestump-agent
left a comment
There was a problem hiding this comment.
Overall this is a really clean design. Reusing the mvdan/sh interpreter that already powers the bash tool is the right call — source, $(...), conditionals, and ${VAR:?...} all come for free, and the ConfigBuilder-on-context gating is an elegant way to keep the same builtins no-op during normal agent bash execution. The test coverage is thorough (per-builtin unit tests + end-to-end tests through the real config.Load pipeline, including precedence and source).
Leaving inline comments for the issues below; none are blockers, but a few are worth addressing before merge.
Docs (nits, but user-facing):
docs/config/README.md:$CURSH_VERSIONtypo defeats the version-gating example entirely.README.mdsecurity note: "Bothcrushrcrun in a full shell" grammar.- The
notificationsoption key is marked "(deprecated)" whilenotification-styleis advertised elsewhere as the replacement, but they write different fields (disable_notificationsvsnotification_style). Worth a sentence clarifying the relationship. - No mention of the empty-resolved-header drop behavior for
--header/--extra-header, which JSON users rely on for env-gated headers.
Design observations (not bugs):
PLAN.mdis a 909-line internal design doc committed to the repo root. It contains stale code sketches (e.g.WithConfigBuilder(context.Background())) that contradict the shipped implementation, plus forward-looking "JSON retirement" and "real-time config" sections. Strongly suggest dropping it from this PR or relocating underdocs/config/.- The same-directory JSON+crushrc warning doesn't name the conflicting keys; for users migrating incrementally, more detail would help.
- There's no
$schema/IDE-autocomplete story forcrushrc, which JSON users will lose when they migrate.
Code nits:
isIntOptionininternal/shellconfig/options.gois dead (always returns false). Drop or wire up.permissions allow+permissions denyinteraction (deny wins viadisabled_tools) isn't documented or tested. A small test would pin the precedence.- The
shellconfig_*_test.gohelpers isolateHOME/XDG_*but don't overrideCRUSH_GLOBAL_CONFIG/CRUSH_GLOBAL_DATA, so a dev/CI with those set could pull in stray global config. The siblingload_test.godoes override them.
Nice work on this — the imperative-map builder that emits one JSON blob per file (rather than per-builtin fragments) is the right evolution, and option reset / provider remove working in-order within a file is a good UX.
|
Good stuff in here, all of which I would have totally missed. Thank you! |
Add Bash-powered config format support (crush.sh). Seven shell builtins (provider, model, mcp, lsp, permissions, hook, options) populate config via JSON fragments stored on a ConfigBuilder in the shell context. Builtins are registered via shell.RegisterBuiltin and gated by context — they are no-ops during normal bash tool execution. Co-Authored-By: Charm Crush <crush@charm.land>
Modify lookupConfigs to find crush.sh and .crush.sh alongside crush.json. Shell config files are executed via shellconfig.LoadShellConfig, which runs the script with config builtins and collects JSON fragments for the existing deep-merge pipeline. Warns when both .json and .sh exist in the same directory. Co-Authored-By: Charm Crush <crush@charm.land>
18 tests covering all builtins (provider, model, mcp, lsp, permissions, hook, options), source includes, conditionals, command substitution, env var expansion, error handling, and full config integration. Also fixes JSON cycle issue in provider and permissions handlers. Co-Authored-By: Charm Crush <crush@charm.land>
Add shellconfig package to architecture diagram, document the crush.sh config format in the key patterns section, and clean up unused helpers. Co-Authored-By: Charm Crush <crush@charm.land>
Each builtin now logs success (slog.Info with entity name) and failure (slog.Error with error detail) using slog, matching the conventions used in the config loader. The loader logs execution start, fragment count, merge results, and completion. Co-Authored-By: Charm Crush <crush@charm.land>
The provider-model builtin lets crush.sh define models that exist on a provider, mapping to the models array in ProviderConfig. Supports --provider (required), --id (required), --name, --context-window, --default-max-tokens, --can-reason, --supports-images, --cost-per-1m-in, --cost-per-1m-out, and --reasoning-effort. Errors are thrown if --provider or --id are missing. Co-Authored-By: Charm Crush <crush@charm.land>
…ns to option provider-model now takes the provider ID as first positional arg (matching provider, mcp, lsp pattern) instead of --provider flag. The options builtin is renamed to option with a key/value syntax: option data-directory .crush, option no-progress, option debug. Negated keys (no-foo) set the corresponding disable_foo field to false. Co-Authored-By: Charm Crush <crush@charm.land>
Replace the confusing "no-" negated option keys with positive keys that invert into the underlying disable_* fields, so users write "option metrics false" instead of the double-negative "disable-metrics" form. Also add a --help-style command reference for the shell config builtins. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Inject the running Crush version into the crush.sh environment so config scripts can detect it and branch behavior, including checking for local "devel" builds. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Each option call appends a single value, so the list keys now read as singular actions (disable-tool, skill-path) instead of plural nouns. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Lets a crush.sh script clear a list option (e.g. skill paths) back to empty, dropping values inherited from a sourced base config while keeping anything added afterward. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Boolean values like TRUE, False, or YES are now recognized regardless of case, matching common shell habits. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Shell config builtins now mutate a shared config model in execution order rather than emitting JSON fragments that are later deep-merged. This matches how a script actually runs and makes clearing and removing values straightforward, eliminating the marker-and-cleanup workarounds. Documents the phased path toward retiring the JSON config format. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Reworks provider and model configuration into a consistent verb-first grammar: providers are added or removed with "provider add"/"provider unset", and models are managed entirely through "model" (add, unset, and large/small selection using the same provider/id form that `crush models` prints). Selecting a slot with no argument prints the current choice. Adding a model requires its provider to already exist, catching typos early. Replaces the separate provider-model command. Co-Authored-By: Charm Crush <crush@charm.land>
Removing a provider or model now uses the more natural "remove" verb, with "rm" as a shorthand, instead of "unset". 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Hooks are now managed with "hook add" and "hook remove" (alias rm), matching the provider and model commands. A hook can be removed by name, or an event's hooks cleared entirely when no name is given. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
LSP servers are now managed with "lsp add" and "lsp remove" (alias rm), matching the provider, model, and hook commands. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land
MCP servers are now managed with "mcp add" and "mcp remove" (alias rm), completing the consistent verb-first grammar across all shell config entity commands. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Permissions are now granted with "permissions allow <tool>...", matching the verb-first style of the other config commands, and repeated tools are deduplicated. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Rewrites the configuration skill to lead with the Bash config format and its verb-first commands, keeping the JSON format as a documented legacy reference with a mapping table between the two. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Adds a conversational, help-style guide for the crush.sh config format and a companion future-work document sketching live, session-only reconfiguration from the bash tool, mirroring the hooks docs. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Anthropic ships built in, so the intro example now sets up a local Ollama provider, which better shows why you'd define a provider and register a model. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Global config locations only ever looked for crush.json, so a global crush.sh (e.g. ~/.config/crush/crush.sh) was silently ignored while a project-level one worked. Each global location now also contributes its crush.sh sibling. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Introduces an external test package that drives config solely through the public LoadShellConfig entry point, starting with permissions: single and multi-tool grants, accumulation, deduplication, and usage errors. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Remove the duplicate option-based spelling so crushrc has one clear way to hide tools from the agent. Existing JSON still uses options.disabled_tools. 💘 Generated with Crush Assisted-by: Crush:openai/gpt-5.6-sol
Add options for selecting the commit trailer style and controlling the Generated with Crush line, including validation and default preservation. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Replace the implementation-oriented cost flags with input, output, cache-create, and cache-hit pricing names while preserving the existing JSON format. Update model examples and verify all four prices survive config load. 💘 Generated with Crush
Add complete UI settings, selected-model tuning and provider options, plus provider discovery, extra-body, and provider-options flags. Preserve model provider options through resolution and retire the obsolete JSON-only docs. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Restructure the config reference around usage, available commands, flags, and short subcommand descriptions so it reads like familiar Cobra help output. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Replace remaining supported JSON examples in the README with crushrc commands, expand option and UI help descriptions, and link the legacy JSON schema. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Restrict global Bash config to the user config directory while keeping the data-directory JSON state load unchanged. Document the config/state boundary and protect it with discovery regression coverage. 💘 Generated with Crush Co-Authored-By: Charm Crush <crush@charm.land>
Convert the sourced file path to forward slashes so the bash interpreter does not treat Windows backslashes as escape characters. 💘 Generated with Crush
Fix $CURSH_VERSION → $CRUSH_VERSION in the crushrc docs and reword the security note so it no longer reads as if two crushrc files run in a shell. 💘 Generated with Crush
Replace the two overlapping options (boolean disable_notifications and string notification_style) with one string field, notifications, that accepts auto, native, osc, bell, or disabled. This removes a confusing double-negative where "notifications false" and "notification-style disabled" touched different fields. Existing configs are migrated automatically on load. 💘 Generated with Crush
Users rely on headers resolving to the empty string being silently omitted for env-gated patterns like --header OpenAI-Organization "$OPENAI_ORG_ID". Document this behavior under provider add and mcp add so it is discoverable. 💘 Generated with Crush
Users migrating incrementally from crush.json to crushrc often keep both files in one directory. The merge warning previously only said both were found, giving no hint about stale duplicates. It now lists the top-level keys defined in both files so leftovers are easy to spot. 💘 Generated with Crush
Documents and tests that "permissions deny" takes precedence over "permissions allow" when both target the same tool, since deny removes the tool from the agent entirely via disabled_tools. 💘 Generated with Crush
Prevents stray global config from polluting test results on machines that set CRUSH_GLOBAL_CONFIG or CRUSH_GLOBAL_DATA. 💘 Generated with Crush
Bring in MCP OAuth support and reconcile it with the bash-based config: resolve conflicts in the shell builtin dispatcher and config ookup tests, and expose the new MCP OAuth fields through the mcp add builtin and docs. 💘 Generated with Crush
Config loads run on the startup and reload critical paths while the config store's write lock is held. A crushrc that blocks (hung command substitution, stray loop) would wedge the whole store indefinitely because LoadShellConfig ran with context.Background() and no deadline. Thread a context through loadFromConfigPaths into LoadShellConfig, and wrap it with a 30s timeout. The reload path passes its real ctx so a cancelled reload interrupts the script; startup passes Background(), still bounded by the internal deadline. Add regression tests: a hanging crushrc respects context cancellation, reload picks up an edited crushrc, a failing crushrc preserves old config, and a hanging crushrc during reload is interruptible without corrupting state.
optionKeyMap, isBoolOption, and isListOption each independently enumerated the same set of keys. Adding a key required touching three switches; missing one caused a silent misparse. Replace all three with a single map[string]optionSpec carrying a kind field (optString, optBool, optList) and an inverted flag. The kind drives parsing, so there is no separate enumeration to drift. Also fix the orphaned doc comment that was physically attached to optionUI instead of optionKeyMap, and move notifications to the string section where it belongs (it was miscategorized under the inverted-bool group and worked only by fall-through).
…e engine providerAdd, mcpAdd, lspAdd, hookAdd, modelAdd, and modelSelect were all the same for-loop-over-switch boilerplate: parse a flag, check the error, wrap it in usage(), assign to the map. ~57 cases across five files, differing only in flag name, parse kind, destination key, and append-vs-assign. Introduce a declarative flagSpec (name, jsonKey, kind, op, optional validate) and a single applyFlags engine. Each builtin now declares its flag surface as a []flagSpec table. Adding a flag is a one-line table entry instead of a five-line copy-paste, and every builtin is consistent by construction. Delete the now-unused flagStr/flagBool/flagInt/flagFloat64/flagKeyValue/ flagJSONObject helpers and the jsonUnmarshal/mergeMap identity wrappers from register.go. Net ~350 lines removed across the package.
The merge warning fired whenever a crush.json and a crushrc coexisted in the same directory, even when they defined disjoint top-level keys. A user intentionally splitting config between the two formats (providers in JSON, options in crushrc) got warned on every load and reload. Only warn when the two files define overlapping keys, naming them. Disjoint coexistence is a supported pattern, not a problem.
jq was hardcoded in a switch inside builtinHandler while every config builtin went through the extraBuiltins map. Two dispatch paths for one concept meant a reader had to know which mechanism a given command used. Register jq via RegisterBuiltin in a shell-package init and collapse the dispatch to a single map lookup. Rename extraBuiltins to builtins now that it holds everything, and initialize the map at declaration instead of a lazy nil check.
The builtin registry had zero coverage: nothing verified that a handler registered via RegisterBuiltin actually gets dispatched through Run, that it shadows a same-named PATH binary, or that unregistered names fall through to the exec layer. Add integration tests for all three, plus an assertion that jq is registered through the shared map rather than a special case.
Three small correctness and clarity fixes surfaced in review: - model add now replaces an existing entry with the same id instead of appending a duplicate, matching the update-in-place behavior of provider add and lsp add. - Document that optionSpecs is intentionally not exhaustive: ui and attribution options are handled as special cases in handleOption. - Add a test pinning that permissions deny writes to options.disabled_tools (not permissions.disabled_tools). The cross-section write is load-bearing for deny-wins-over-allow and had no coverage at the shellconfig level.
CaptureStalenessSnapshot was only handed the paths that successfully loaded, so a config file that did not exist at load time was never tracked. A global crushrc created after startup went unnoticed by the staleness check until something else triggered a reload. Pass the union of discovered paths (from lookupConfigs) and loaded paths to the snapshot capture in both Load and reloadFromDiskLocked. Non-existent paths are recorded with Exists:false so their later creation is detected as a change. Add a regression test covering a global crushrc created after startup.
The three existing load benchmarks only cover JSON parsing. crushrc execution (shell interpreter + config builtins + JSON marshal) is now on the startup and reload critical path but had no performance tracking, so a regression there would go unnoticed. Add BenchmarkLoadFromConfigPaths_ShellConfig exercising a realistic crushrc with providers, model selection, permissions, and options.
- Fix "forseeable" -> "foreseeable" and two grammar slips in the config README. - Document that when a tool appears in both permissions allow and deny, deny wins. The behavior was implemented and tested but never stated in the user-facing docs. - List the valid notifications enum values in the crush-config skill so the agent can write correct config without guessing.
This revision adds imperative, Bash-based configuration to Crush (don't worry, JSON is still supported for now). This makes configuration a lot more flexible than the current JSON format for a few reasons:
Full docs here.
Why did we choose Bash? Because Crush ships with a glorious first class, fully cross platform pure Go bash interpreter, and LLMs are great at Bash. The larger goal, however, is to allow Crush to imperatively change it's config, in realtime, with the same commands used to configure it.
Thus, the configuration itself is done via a series of builtin commands. Here are some things you can do:
It's also composable:
And can include logic:
The configuration lives globally at
~/.config/crush/crushrcand locallly at./.crushrcandcrushrc.As mentioned, JSON will still work as normal with one small change:
Note that both
crushrcandcrush.jsonare backed with the Bash interpreter and both are trusted code, and should be treated with the same precautions as any other script.