From ef8cede35e0bf6934ff6cf288482c88c7d44cfde Mon Sep 17 00:00:00 2001 From: Jean Mertz Date: Thu, 19 Mar 2026 16:40:02 +0100 Subject: [PATCH] docs(rfd): Add RFDs 055, 056, 057 for tool groups and group config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Introduce three new RFDs covering the design of tool groups and group-level configuration: - RFD 055 defines named tool groups with CLI shorthand (`--tools GROUP` / `--no-tools GROUP`) and exhaustive validation, ensuring every tool is classified relative to a group marked `exhaustive = true`. - RFD 056 extends groups with a `defaults` section, allowing member tools to inherit shared configuration (`run`, `style`, etc.) without per-tool repetition. - RFD 057 adds an `overrides` section to groups, enforcing configuration that takes priority over tool-level settings — useful for policy enforcement via `--cfg` files. Also relaxes the `rfd-extend` justfile rule to allow extending RFDs in Discussion status (previously only Accepted or Implemented), which unblocks chaining these RFDs before any of them are accepted. Signed-off-by: Jean Mertz --- docs/.vitepress/rfd-summaries.json | 12 + docs/rfd/055-tool-groups.md | 413 ++++++++++++++++++ docs/rfd/056-group-configuration-defaults.md | 256 +++++++++++ docs/rfd/057-group-configuration-overrides.md | 258 +++++++++++ justfile | 6 +- 5 files changed, 942 insertions(+), 3 deletions(-) create mode 100644 docs/rfd/055-tool-groups.md create mode 100644 docs/rfd/056-group-configuration-defaults.md create mode 100644 docs/rfd/057-group-configuration-overrides.md diff --git a/docs/.vitepress/rfd-summaries.json b/docs/.vitepress/rfd-summaries.json index 9db2e8c3..62b4930e 100644 --- a/docs/.vitepress/rfd-summaries.json +++ b/docs/.vitepress/rfd-summaries.json @@ -214,5 +214,17 @@ "054-split-conversation-config-and-events.md": { "hash": "716d382ca9c81c76a5b3760f6727ba3ac44bdc049d4eb366d29366f83e1f535a", "summary": "Split conversation events.json into separate base_config.json and events.json files for better readability and clarity." + }, + "055-tool-groups.md": { + "hash": "015a91d70cdf68a5c8b13756c237bd435d9e8d0b6bc5fb9d1e9b9ebf2e3d2a9d", + "summary": "Named tool groups enable CLI shortcuts and exhaustive validation ensuring every tool is classified relative to a group." + }, + "056-group-configuration-defaults.md": { + "hash": "79a4a58c5b89474ac1267d4e156e48a01282d7ed7400a5ad857ef88af587cfad", + "summary": "Groups can declare default tool configuration, allowing tool inheritance without per-tool repetition." + }, + "057-group-configuration-overrides.md": { + "hash": "002459492c526bf9cf295b94aecb3bfb79d544d15aaea44dfc7c5c8f4a5e8eb6", + "summary": "Adds group `overrides` section to enforce tool configuration that outlasts tool-level settings but yields to CLI flags." } } diff --git a/docs/rfd/055-tool-groups.md b/docs/rfd/055-tool-groups.md new file mode 100644 index 00000000..3f22183f --- /dev/null +++ b/docs/rfd/055-tool-groups.md @@ -0,0 +1,413 @@ +# RFD 055: Tool Groups + +- **Status**: Discussion +- **Category**: Design +- **Authors**: Jean Mertz +- **Date**: 2025-07-20 +- **Extended by**: [RFD 056](056-group-configuration-defaults.md) + +## Summary + +This RFD introduces tool groups: named sets of tools that can be enabled or +disabled as a unit via `--tools GROUP` / `--no-tools GROUP`. Groups support +exhaustive validation, ensuring every tool is classified relative to a group. +Group membership is declared on the tool side via a `groups` field. + +## Motivation + +Tools are configured individually in `conversation.tools.NAME`. Named config +fragments (loaded via `--cfg` or the `extends` field) bundle tool configurations +into reusable files, but there is no way to enable or disable a *set* of tools +from the CLI. + +Today, disabling all filesystem write tools requires: + +```sh +jp query -T fs_create_file,fs_modify_file,fs_delete_file,fs_move_file "explain this code" +``` + +And ensuring that *every* tool in the system is classified as "write" or "not +write" is impossible — there is no validation mechanism. A new MCP tool could +silently retain write access when the user expected `--no-tools write` to +disable all write-capable tools. + +A `--cfg disable_write_tools.toml` workaround exists, but it's fragile — you +maintain a separate file that mirrors tool names, and nothing validates that the +file stays in sync when tools are added or removed. + +Tool groups solve two problems: + +1. **CLI shorthand**: `--no-tools write` disables all write tools in one flag. +2. **Exhaustive validation**: a group marked `exhaustive = true` guarantees that + every loaded tool has been explicitly classified relative to that group. No + tool escapes unreviewed. + +## Design + +### Group Definition + +Groups are defined at `conversation.tools.groups.NAME`: + +```toml +[conversation.tools.groups] +write = { exhaustive = true } +read = {} +git = {} +cargo = {} +``` + +A group definition contains: + +- **`exhaustive`** (optional, default `false`): whether every tool must be + classified relative to this group. See [Exhaustive + Validation](#exhaustive-validation). + +Groups do not list their members. Membership is declared on the tool side (see +below). + +Group names must not collide with tool names. If a group and a tool share a +name, JP exits with a config error at startup. + +Group names must not start with `!` (reserved for the exclusion shorthand). + +### Tool-Side Group Membership + +Tools declare their group memberships via a `groups` field: + +```toml +[conversation.tools.fs_create_file] +groups = ["write"] + +[conversation.tools.fs_read_file] +groups = ["read", "!write"] +``` + +Each entry in the array is either: + +- A **string** — shorthand for included membership. +- A **`!`-prefixed string** — shorthand for excluded membership. +- A **structured entry** — `{ group = "NAME", membership = "include" | "exclude" + }` for when the long form is preferred. + +The three forms are equivalent: + +```toml +# These all mean "included in write": +groups = ["write"] +groups = [{ group = "write" }] +groups = [{ group = "write", membership = "include" }] + +# These all mean "excluded from write": +groups = ["!write"] +groups = [{ group = "write", membership = "exclude" }] +``` + +Referencing a group name that does not exist in `conversation.tools.groups` is a +config error. + +Naming both `!write` and `write` is valid, last-defined wins. + +#### Membership States + +For any (tool, group) pair, there are exactly three states: + +| State | Meaning | How expressed | +|---------------|---------------------------------|------------------------------------------| +| **Included** | Tool is a member of the group | `"write"` or `{ group = "write" }` | +| **Excluded** | Tool is explicitly not a member | `"!write"` or `{ group = "write", | +| | | membership = "exclude" }` | +| **Undefined** | Tool has not been classified | Group not mentioned in `groups` | + +The distinction between excluded and undefined is what makes exhaustive +validation meaningful. + +### Interaction with `*` Defaults + +The `*` defaults section can set a default `groups` field: + +```toml +[conversation.tools.'*'] +groups = ["write"] + +[conversation.tools.fs_read_file] +groups = ["!write", "read"] +``` + +This establishes a fail-closed baseline: all tools default to the write group. A +tool that is *not* a write tool must explicitly exclude itself. This means +`--no-tools write` is safe by default — if a new tool is added and the author +forgets to classify it, it lands in the write group and gets disabled when write +tools are disabled. No tool silently escapes with write access. + +The `groups` field uses **merge-by-group-name** semantics at every merge +boundary — between `*` defaults and tool-level config, and between config file +layers (workspace config, `--cfg` overrides, etc.). + +The merge rule is the same at every boundary: + +1. Start with the lower-priority groups array. +2. Remove any entries whose group name appears in the higher-priority array (the + higher-priority side overrides those). +3. Append all higher-priority entries. + +Retained lower-priority entries come first. Higher-priority entries follow in +their declared order. + +This means a `--cfg` override that sets `groups = ["!write"]` on a tool will +override the `write` entry from the workspace config's `*` defaults, while +retaining any other group entries that were already present. + +**Example:** + +```toml +# Defaults +[conversation.tools.'*'] +groups = ["write"] + +# fs_read_file overrides write, adds read +[conversation.tools.fs_read_file] +groups = ["!write", "read"] + +# github_issues inherits "write" from *, adds github +[conversation.tools.github_issues] +groups = ["github"] + +# cargo_check: inherits "write" from *, no tool-level groups override +# (if it has no groups field at all, it inherits ["write"] from *) + +# Effective groups: +# fs_read_file: ["!write", "read"] (tool replaced *'s write with !write, added read) +# github_issues: ["write", "github"] (*'s write retained, tool's github appended) +# cargo_check: ["write"] (inherited from *) +``` + +This merge behavior avoids the verbosity problem with exhaustive groups. Without +it, every tool that sets its own `groups` would need to re-declare all +exhaustive group memberships — exactly the kind of error-prone repetition that +exhaustive validation is meant to prevent. + +### Exhaustive Validation + +A group with `exhaustive = true` requires that every enabled tool has been +classified relative to that group — either included or excluded. If any tool has +the group in undefined state (not mentioned in the tool's effective `groups` +array after `*` merge), JP exits with a startup error. + +```toml +[conversation.tools.groups.write] +exhaustive = true + +[conversation.tools.'*'] +groups = ["write"] # baseline: all tools in write + +[conversation.tools.fs_read_file] +groups = ["!write", "read"] # override: excluded from write + +[conversation.tools.some_new_mcp_tool] +source = "mcp.my_server" +# inherits "write" from * → classified → OK +``` + +Without the `*` baseline, every tool must explicitly mention the exhaustive +group in its `groups` array. Both approaches are valid — the `*` baseline is a +convenience for the common pattern where most tools share a default +classification. + +Exhaustive validation runs after all config layers are merged (config files, +`--cfg`, CLI flags) but before the query starts. + +### CLI Interaction + +`--tools` (`-t`) and `--no-tools` (`-T`) accept group names in addition to tool +names: + +```sh +# Enable all tools in the "read" and "cargo" groups +jp query -t read,cargo "fix the parser" + +# Enable all tools, then disable the "write" group +jp query -t -T write "explain this code" + +# Disable the "git" group +jp query -T git "review this design" +``` + +Since group names and tool names cannot collide, name resolution is unambiguous. + +`--tools GROUP` enables all tools with **included** membership in that group. +`--no-tools GROUP` disables all tools with included membership in that group. + +### Interaction with Tool `enable` Field + +The existing `enable` field on tool config controls tool activation behavior. +Groups interact with it as follows: + +| `enable` value | `--tools GROUP` | `--no-tools GROUP` | +|---------------------|-------------------|----------------------| +| `None` (default) | Enabled | Disabled | +| `on` / `true` | Enabled | Disabled | +| `off` / `false` | Enabled | Disabled | +| `explicit` | **Not enabled** | Disabled | +| `explicit_or_group` | Enabled | Disabled | +| `always` | (already enabled) | **Not disabled** | + +- **`None` (default)**: the tool has no explicit enable setting. Treated as + enabled (consistent with `is_none_or(Enable::is_on)` in the current + implementation). + +- **`explicit`**: not enabled by `--tools GROUP`, consistent with not being + enabled by bare `--tools`. Requires being named directly: `--tools TOOL`. + +- **`explicit_or_group`** (new variant): like `explicit`, the tool is not + enabled by bare `--tools`. But unlike `explicit`, it *is* enabled when a group + it belongs to is activated via `--tools GROUP`. This allows tools that should + not be swept up by blanket enable-all but should respond to targeted group + activation. + +- **`always`**: cannot be disabled by any mechanism, including `--no-tools + GROUP`. Consistent with existing behavior for system-critical tools like + `describe_tools`. + +## Drawbacks + +**Merge-by-name is a special case.** The `groups` field uses merge-by-name +semantics with `*` defaults, while scalar fields use replace semantics and other +array fields use `MergeableVec`'s append/replace strategies. This must be +documented clearly. The justification (avoiding re-declaration verbosity with +exhaustive groups) is sound, but it adds a concept users must learn. + +**New `Enable` variant.** Adding `explicit_or_group` increases the surface area +of an already nuanced enum. Users who don't use groups will never encounter it, +but it is another value to document and maintain. + +## Alternatives + +### Include/exclude lists on group definitions + +Instead of tool-side membership, groups could declare their members: + +```toml +[conversation.tools.groups.write] +include = ["fs_create_file", "fs_modify_file"] +exclude = ["fs_read_file", "cargo_check"] +``` + +This was rejected because it creates dual membership paths — membership +expressed both on the group and on the tool. Conflicts between the two require +resolution rules. Tool-side declaration is the single source of truth. + +### Tags instead of groups + +Tools could declare tags (`tags = ["read", "write"]`) and the CLI could filter +by tag (`--tools @read`). This is similar to the chosen design but does not +support exhaustive validation or the include/exclude distinction. + +### `exhaustive = "include" | "exclude"` variants + +Instead of just `true | false`, exhaustive could default unclassified tools to +included or excluded. These were dropped because `"include"` is functionally +equivalent to `false` (the check never fires — every tool auto-classifies) and +`"exclude"` is already the natural behavior of the `*` defaults pattern +(`'*'.groups = ["write"]` with per-tool `"!write"` overrides). The boolean is +sufficient. + +## Non-Goals + +- **Group-level configuration.** Groups in this RFD are purely membership + containers for CLI shorthand and exhaustive validation. Allowing groups to + carry config defaults or overrides (e.g., group-wide `run` or `style` + settings) is a natural extension but is deferred to a future RFD. +- **Recursive groups.** Groups cannot contain other groups. The real-world + taxonomy is shallow (read, write, git, cargo, github). If deeper nesting is + needed, it can be added in a future RFD. + +## Risks and Open Questions + +**`*` merge semantics may surprise users.** The merge-by-name behavior for +`groups` differs from other fields. If this causes confusion in practice, we +could fall back to replace semantics and accept the verbosity cost for +exhaustive groups. + +**`Enable` enum growth.** The enum now has five variants (`on`, `off`, +`explicit`, `explicit_or_group`, `always`). If future features add more +activation modes, the enum may need restructuring. For now, five variants is +manageable. + +## Implementation Notes + +### `GroupMemberships` type + +The merge-by-group-name behavior is implemented as a dedicated +`GroupMemberships` type rather than extending `MergeableVec` with a third +strategy. The key extraction logic (parsing group names from strings, +`!`-prefixed strings, and structured entries) is specific to the groups type and +doesn't generalize to other vec fields. + +`GroupMemberships` wraps a `Vec` and provides: + +- `merge_from(&mut self, other: &GroupMemberships)` — the merge-by-name + operation. +- `included(&self) -> impl Iterator` — iterate included group names + in declaration order. +- `is_classified(&self, group: &str) -> bool` — check if a group is mentioned + (included or excluded), for exhaustive validation. + +`GroupEntry` is an enum supporting string shorthand (`"write"`, `"!write"`) and +structured form (`{ group, membership }`), with `group_name()` and +`is_excluded()` accessors. + +The type uses `#[serde(transparent)]` to serialize as a plain JSON/TOML array. A +custom schematic merge function (`merge_group_memberships`) integrates it with +the config system. + +## Implementation Plan + +### Phase 1: Types and membership + +1. Implement `GroupMemberships`, `GroupEntry`, and `Membership` types with + merge-by-group-name semantics and the `merge_group_memberships` merge + function. +2. Add `ToolGroupConfig` struct with `exhaustive` field (no config sections). +3. Add `groups: IndexMap` to `ToolsConfig` (as a named + field before the flattened `tools` field, following the same pattern as + `defaults`). +4. Add `groups` field to both `ToolConfig` and `ToolsDefaultsConfig`, using the + `GroupMemberships` type. +5. Update `AssignKeyValue`, `PartialConfigDelta`, and `ToPartial` + implementations for `PartialToolsConfig` to handle the new `groups` field. +6. Validate at config load time: no group/tool name collisions, no references to + undefined groups. + +### Phase 2: Exhaustive validation + +1. After full config merge, iterate all enabled tools and compute their + effective groups (tool-level merged with `*` defaults). +2. For each group with `exhaustive = true`, verify every enabled tool is + classified (included or excluded). Exit with an error listing unclassified + tools if any are found. + +### Phase 3: CLI integration + +1. Extend `--tools` / `--no-tools` name resolution to check groups (unambiguous + since group/tool name collisions are a config error). +2. When a group name is matched, expand to enable/disable all tools with + included membership in that group. +3. Respect `enable` field variants: `explicit` tools are not activated by group + enable; `always` tools are not disabled by group disable. +4. Add `Enable::ExplicitOrGroup` variant. +5. Update existing tests in `query_tests.rs` to cover group interactions. + +## References + +- [Configuration architecture] — progressive complexity design principles +- `MergeableVec` (`crates/jp_config/src/types/vec.rs`) — existing array merge + strategies in the config system +- `crates/jp_config/src/conversation/tool.rs` — `ToolConfig`, `ToolsConfig`, + `ToolsDefaultsConfig`, `Enable` +- `crates/jp_cli/src/cmd/query.rs` — `apply_enable_tools`, `--tools` / + `--no-tools` handling +- [RFD 042] — tool options (per-tool runtime configuration, complementary + feature) + +[Configuration architecture]: ../configuration.md +[RFD 042]: 042-tool-options.md diff --git a/docs/rfd/056-group-configuration-defaults.md b/docs/rfd/056-group-configuration-defaults.md new file mode 100644 index 00000000..fd72b7dc --- /dev/null +++ b/docs/rfd/056-group-configuration-defaults.md @@ -0,0 +1,256 @@ +# RFD 056: Group Configuration Defaults + +- **Status**: Discussion +- **Category**: Design +- **Authors**: Jean Mertz +- **Date**: 2025-07-20 +- **Extends**: [RFD 055](055-tool-groups.md) +- **Extended by**: [RFD 057](057-group-configuration-overrides.md) + +## Summary + +This RFD adds a `defaults` section to tool group definitions, allowing groups to +carry fallback configuration (`enable`, `run`, `result`, `style`, `questions`) +for their member tools. The tool's `groups` array ordering determines which +group's defaults take priority. + +## Motivation + +[RFD 055] introduces tool groups for CLI shorthand and exhaustive validation, +but groups carry no configuration — they are purely membership containers. +Configuring shared behavior across a set of tools still requires per-tool +repetition: + +```toml +[conversation.tools.git_commit] +run = "unattended" +style.inline_results = "off" + +[conversation.tools.git_diff] +run = "unattended" +style.inline_results = "off" + +[conversation.tools.git_add_intent] +run = "unattended" +style.inline_results = "off" +``` + +A `--cfg` file can bundle these together, but the duplication remains — each +tool repeats the same fields. If the group's intended behavior changes, every +tool must be updated. + +Group defaults solve this: declare the shared configuration once on the group, +and member tools inherit it automatically. + +```toml +[conversation.tools.groups.git] +defaults.run = "unattended" +defaults.style.inline_results = "off" +``` + +Tools in the `git` group inherit `run = "unattended"` and `style.inline_results += "off"` without repeating them. A tool can still override any field in its own +config. + +## Design + +### `defaults` Section + +Groups gain an optional `defaults` section nested under the group definition: + +```toml +[conversation.tools.groups.write] +exhaustive = true +defaults.run = "ask" + +[conversation.tools.groups.git] +defaults.run = "unattended" +defaults.style.inline_results = "off" +defaults.style.results_file_link = "off" + +[conversation.tools.groups.read] +# no defaults — membership-only group +``` + +The `defaults` namespace prevents collisions between group-level fields (like +`exhaustive`) and tool config fields (like `run`). + +### Supported Fields + +The `defaults` section accepts the behavioral subset of tool config fields: + +| Field | Type | Description | +|-------------|------------------------------------|-----------------------------| +| `enable` | `Enable` | Whether the tool is enabled | +| `run` | `RunMode` | How to run the tool | +| `result` | `ResultMode` | How to deliver tool results | +| `style` | `DisplayStyleConfig` | Terminal display settings | +| `questions` | `IndexMap` | Question routing defaults | + +Fields excluded (per-tool identity, not behavioral): `source`, `command`, +`summary`, `description`, `examples`, `parameters`, `options`. + +The struct reuses the same types as `ToolsDefaultsConfig` (with `questions` +added), so existing serialization, assignment, and delta logic applies. + +### Merge Chain + +Group defaults sit between `*` defaults and tool-level config in the merge +chain: + +```txt +* defaults > group[0].defaults > group[1].defaults > ... > tool config > CLI flags +``` + +Where `group[0]`, `group[1]`, etc. are the **included** groups from the tool's +effective `groups` array, in declaration order. Excluded groups (`!write`) do +not contribute defaults. + +This follows the same last-write-wins principle used throughout JP's config +system: later layers override earlier layers, and tool config overrides all +group defaults. + +**Example:** + +```toml +[conversation.tools.groups.write] +exhaustive = true +defaults.run = "ask" + +[conversation.tools.groups.verbose] +defaults.style.inline_results = "full" + +[conversation.tools.'*'] +groups = ["write"] + +[conversation.tools.fs_modify_file] +groups = ["write", "verbose"] +# Chain: * > write.defaults > verbose.defaults > tool config +# run = "ask" (from write), inline_results = "full" (from verbose) +# Tool can override either field in its own config. + +[conversation.tools.fs_read_file] +groups = ["!write", "read"] +# Chain: * > read.defaults > tool config +# write.defaults does NOT apply (excluded). +``` + +### Resolution in `ToolConfigWithDefaults` + +`ToolConfigWithDefaults` currently resolves fields via simple fallback: + +```rust +pub fn run(&self) -> RunMode { + self.tool.run.unwrap_or(self.defaults.run) +} +``` + +Group defaults extend this pattern. The accessor walks `tool > groups > +defaults`, returning the first value found: + +```rust +pub fn run(&self) -> RunMode { + self.tool.run + .or_else(|| self.groups.iter().rev().find_map(|g| g.run)) + .unwrap_or(self.defaults.run) +} +``` + +`ToolConfigWithDefaults` gains a `groups` field storing the included groups' +defaults in declaration order. The constructor stores inputs without mutating +them — `tool` always represents what the tool actually declared: + +```rust +pub struct ToolConfigWithDefaults { + /// The tool configuration (unmodified). + tool: ToolConfig, + + /// Included group defaults, in declaration order. + groups: Vec, + + /// The global defaults. + defaults: ToolsDefaultsConfig, +} +``` + +This keeps the lazy fallback pattern that already exists for `* defaults` and +extends it with one additional layer. The `tool` field remains a clean +representation of the tool's own config, which makes provenance tracing +straightforward (compare `tool.run` vs `groups[n].run` vs `defaults.run`). + +### Interaction with `*` Defaults + +Group defaults and `*` defaults are complementary, not competing: + +- `*` provides the universal baseline for all tools. +- Group defaults provide a per-group baseline for member tools. +- Tool config provides per-tool specifics. + +A tool in the `git` group resolves `run` as: `tool.run > git.defaults.run > +*.run`. If the tool doesn't set `run` and neither does the `git` group, the `*` +default applies. + +## Drawbacks + +**Deeper merge chain.** The chain grows from `* > tool` to `* > groups > tool`. +For a tool in multiple groups, each group is a layer. In practice most tools +will be in one or two groups, but debugging "where did this value come from?" +becomes harder until `jp config show` gains merge-chain tracing. + +## Alternatives + +### Eager merge at construction time + +Instead of lazy resolution in accessors, the constructor could clone the tool +config and fold group defaults into it, so accessors stay as +`self.tool.run.unwrap_or(self.defaults.run)`. This was rejected because it +mutates the `tool` field — making it impossible to distinguish "the tool set +this" from "a group set this" without storing a separate unmerged copy. + +### Reuse `ToolsDefaultsConfig` directly + +The group defaults struct could be exactly `ToolsDefaultsConfig`. This was +rejected because `ToolsDefaultsConfig` uses required fields (e.g., +`#[setting(required)] pub run: RunMode`) that make sense for global defaults +(every config must provide a run mode) but not for group defaults (a group may +only want to set `style`, leaving `run` unspecified). Group defaults need all +fields optional. + +## Non-Goals + +- **Group overrides.** This RFD covers `defaults` only — configuration that + tool-level config can override. A future RFD may add an `overrides` section + where group config takes priority over tool-level config, enabling + persona-level policy enforcement. +- **`jp config show` tracing.** Displaying which layer set which value is useful + for debugging group defaults but is orthogonal to this RFD. + +## Implementation Plan + +Depends on [RFD 055] (tool groups with membership and exhaustive validation). + +### Phase 1: Group defaults type + +1. Add `ToolGroupDefaults` struct with all-optional fields: `enable`, `run`, + `result`, `style`, `questions`. Implement `AssignKeyValue`, + `PartialConfigDelta`, and `ToPartial`. +2. Add `defaults: ToolGroupDefaults` to `ToolGroupConfig`. + +### Phase 2: Lazy resolution in `ToolConfigWithDefaults` + +1. Add `groups: Vec` field to `ToolConfigWithDefaults`. +2. Update `ToolsConfig::get()` and `ToolsConfig::iter()` to look up the tool's + included groups and pass their defaults when constructing + `ToolConfigWithDefaults`. +3. Update accessor methods (`run()`, `result()`, `style()`, `enable()`, + `enable_mode()`, `questions()`) to walk `tool → groups → defaults`. + +## References + +- [RFD 055] — tool groups (membership, exhaustive validation, CLI integration) +- [Configuration architecture] — progressive complexity design principles +- `crates/jp_config/src/conversation/tool.rs` — `ToolConfig`, + `ToolConfigWithDefaults`, `ToolsDefaultsConfig` + +[RFD 055]: 055-tool-groups.md +[Configuration architecture]: ../configuration.md diff --git a/docs/rfd/057-group-configuration-overrides.md b/docs/rfd/057-group-configuration-overrides.md new file mode 100644 index 00000000..6d9c187f --- /dev/null +++ b/docs/rfd/057-group-configuration-overrides.md @@ -0,0 +1,258 @@ +# RFD 057: Group Configuration Overrides + +- **Status**: Discussion +- **Category**: Design +- **Authors**: Jean Mertz +- **Date**: 2025-07-20 +- **Extends**: [RFD 056](056-group-configuration-defaults.md) + +## Summary + +This RFD adds an `overrides` section to tool group definitions. Unlike +`defaults` (which tool-level config can override), `overrides` enforces +configuration that takes priority over tool-level settings. Only CLI flags can +override group overrides. + +## Motivation + +[RFD 056] adds group defaults — fallback config that tools can override. This +covers the common case of reducing repetition. But a different use case remains +unsolved: **group-level config enforcement**. + +Consider a workflow where tools are configured per-tool in the workspace config, +and a config file loaded via `--cfg` needs to guarantee certain behavior: + +```toml +# Workspace config: tools set their own run mode +[conversation.tools.fs_modify_file] +run = "unattended" + +[conversation.tools.fs_create_file] +run = "unattended" +``` + +```toml +# review.toml — loaded via --cfg +# Goal: force all write tools to ask before running, regardless of per-tool settings. +# Problem: group defaults can't do this — tool-level "unattended" overrides them. +``` + +With `defaults.run = "ask"` on the write group, each tool's `run = "unattended"` +wins (tool config overrides group defaults). The config cannot enforce its +policy. + +Group overrides solve this: they sit *after* tool-level config in the merge +chain, so the config's `run = "ask"` takes priority over per-tool settings. + +```toml +# review.toml +[conversation.tools.groups.write] +overrides.run = "ask" +``` + +## Design + +### `overrides` Section + +Groups gain an optional `overrides` section, alongside the existing `defaults`: + +```toml +[conversation.tools.groups.write] +exhaustive = true +defaults.run = "ask" # fallback if tool doesn't set run + +[conversation.tools.groups.safety] +overrides.run = "ask" # enforced regardless of tool config +``` + +Both sections accept the same fields (see [RFD 056] for the field list): +`enable`, `run`, `result`, `style`, `questions`. + +A group can use `defaults`, `overrides`, or both: + +```toml +[conversation.tools.groups.safety] +defaults.style.inline_results = "full" # tool can customize display +overrides.run = "ask" # tool cannot skip confirmation +``` + +### Merge Chain + +The full merge chain with both defaults and overrides: + +```txt +* > group[0..n].defaults > tool config > group[0..n].overrides > CLI flags +``` + +Overrides sit between tool config and CLI flags. This means: + +- Tool config cannot override them (that's the point). +- CLI flags can still override them (CLI authority is preserved). + +Within the overrides position, group ordering follows the tool's `groups` array +— later groups have higher priority, same as defaults. + +**Example:** + +```toml +[conversation.tools.groups.dev] +overrides.enable = true +overrides.run = "unattended" + +[conversation.tools.groups.safety] +overrides.run = "ask" + +[conversation.tools.cargo_check] +groups = ["dev"] +run = "unattended" +# Chain: * > dev.defaults > tool (run=unattended) > dev.overrides (run=unattended) > CLI +# Both tool and override agree. Effective run = "unattended". + +[conversation.tools.fs_modify_file] +groups = ["dev", "safety"] +run = "unattended" +# Chain: * > dev.defaults > safety.defaults > tool (run=unattended) +# > dev.overrides (run=unattended) > safety.overrides (run=ask) → CLI +# safety.overrides wins over both tool config and dev.overrides. +# Effective run = "ask". +``` + +### Force-Enabling via `enable` + +A common use case is a `--cfg` file that force-enables a group of tools: + +```toml +# dev-tools.toml +[conversation.tools.groups.dev] +overrides.enable = true +``` + +This overrides per-tool `enable` settings. A tool with `enable = false` in the +workspace config becomes enabled when the config file is loaded. CLI flags can +still disable it: + +```sh +# Config file force-enables dev tools, but CLI can override +jp query -c dev-tools.toml -t dev -T fs_modify_file "review this" +``` + +### `apply_enable_tools` Group Awareness + +The `apply_enable_tools` function in `query.rs` currently inspects +`PartialToolConfig.enable` to determine CLI behavior. With group overrides, a +tool's effective enable may come from a group's `overrides.enable`, not the +tool's own field. + +The CLI enable/disable logic must resolve each tool's effective enable through +the full chain (`* > group.defaults > tool > group.overrides`) before applying +`--tools` / `--no-tools` flags. + +## Drawbacks + +**Five-position merge chain.** `* > group.defaults > tool > group.overrides > +CLI` is the deepest merge chain in the config system. Debugging "where did this +value come from?" requires understanding all five positions. This cost is +justified by the config enforcement use case but should be mitigated by future +`jp config show` tracing. + +**Override power.** A `--cfg` file with group overrides can silently change tool +behavior in ways that are hard to trace. A misconfigured override (e.g., +`overrides.enable = false` on a group containing important tools) could cause +subtle issues. The mitigation is that CLI flags always win, so the user retains +final authority. + +**Two config sections per group.** Users must understand the difference between +`defaults` and `overrides`. Most groups will only use `defaults` — the +`overrides` section is a power-user feature. But its existence means users +encounter a design decision when creating groups. + +## Alternatives + +### Group-level `mode` instead of two sections + +Instead of separate `defaults` and `overrides`, a group could carry a single +`config` section with a `mode` field: + +```toml +[conversation.tools.groups.safety] +mode = "override" +config.run = "ask" +config.style.hidden = false +``` + +This was rejected because it's all-or-nothing: if you want `run` to override but +`style` to be a default, you need two separate groups. The +`defaults`/`overrides` split gives per-field control without per-field syntax. + +### Per-field priority flags + +Each value could carry its own priority annotation: + +```toml +[conversation.tools.groups.safety] +config.run = { value = "ask", priority = "override" } +``` + +Maximum flexibility, but every field becomes a tagged union. The config syntax +gets ugly, the implementation is complex, and the mental model is harder. + +## Non-Goals + +- **`jp config show` tracing.** Displaying which layer (defaults vs overrides vs + tool) set which value. Useful but orthogonal. +- **Override-of-override resolution.** If two `--cfg` files both define + overrides for the same group, normal config layer merging applies (later + `--cfg` wins). No special conflict resolution is needed. + +## Risks and Open Questions + +**Interaction with `enable = "explicit"`.** If a group has `overrides.enable = +"explicit"`, the tool won't respond to `--tools GROUP` (per the enable table in +[RFD 055]). This is internally consistent but surprising — the group's own +override prevents group activation. This should be documented: use +`explicit_or_group` if you want the tool to be hidden by default but still +respond to group activation. + +## Implementation Plan + +Depends on [RFD 056] (group configuration defaults). + +### Phase 1: Overrides type and merge chain + +1. Add `overrides: ToolGroupDefaults` to `ToolGroupConfig` (reuses the same + struct as `defaults`). +2. Add `overrides: Vec` to `ToolConfigWithDefaults` + (alongside the existing `groups` field for defaults). +3. Update `ToolsConfig::get()` and `ToolsConfig::iter()` to pass group overrides + when constructing `ToolConfigWithDefaults`. +4. Update accessor methods to resolve through the full chain. Overrides take + priority over tool config: + + ```rust + pub fn run(&self) -> RunMode { + // Overrides win over tool config + self.overrides.iter().rev().find_map(|g| g.run) + // Then tool config + .or(self.tool.run) + // Then group defaults + .or_else(|| self.groups.iter().rev().find_map(|g| g.run)) + // Then * defaults + .unwrap_or(self.defaults.run) + } + ``` + +### Phase 2: CLI integration + +1. Update `apply_enable_tools` to resolve each tool's effective enable through + the full chain (`* → group.defaults → tool → group.overrides`) before + applying CLI flags. + +## References + +- [RFD 055] — tool groups (membership, exhaustive validation, CLI integration) +- [RFD 056] — group configuration defaults +- `crates/jp_cli/src/cmd/query.rs` — `apply_enable_tools` +- `crates/jp_config/src/conversation/tool.rs` — `ToolConfigWithDefaults` + +[RFD 055]: 055-tool-groups.md +[RFD 056]: 056-group-configuration-defaults.md diff --git a/justfile b/justfile index 7493cb08..bea15c72 100644 --- a/justfile +++ b/justfile @@ -291,13 +291,13 @@ rfd-extend NNN MMM: echo "No RFD found with number ${new_num}." >&2; exit 1 fi - # Validate that the extended RFD is in Accepted or later status. + # Validate that the extended RFD is in Discussion or later status. old_status=$(sed -n 's/^- \*\*Status\*\*: \(.*\)/\1/p' "$old_file" | head -1) case "$old_status" in - Accepted|Implemented) ;; + Discussion|Accepted|Implemented) ;; *) echo "Cannot extend RFD ${old_num} (status: '${old_status}')." >&2 - echo "Only Accepted or Implemented RFDs can be extended." >&2 + echo "Only Discussion, Accepted or Implemented RFDs can be extended." >&2 exit 1 ;; esac