fix(config): hint at removed fields instead of re-adding them

CHANGELOG.md

@@ -7,6 +7,10 @@ All notable changes to this project will be documented in this file.
 
 This release improves safety classification for background jobs and shell commands, enhances the `--key` option for share commands, and advances the internal config schema to v16.
 
+## Breaking Changes
+
+- The `safer` boolean flag removed from the shell toolset (see Technical Changes below) is now rejected outright: a config that still sets `safer: true` on a shell toolset fails to load with `unknown field "safer"` instead of loading silently, including version-less configs (which resolve to the latest schema). Delete the flag — it has had no effect since v1.117.0, superseded by session-wide [safety modes](https://github.com/docker/docker-agent/blob/main/examples/safety_modes.yaml). Pinning `version: "14"` (or lower) is not the fix, since frozen schema versions are not maintained long-term; the field must be removed from the YAML. The load-time error now includes a hint naming the last config version that accepted the field.
+
 ## What's New
 
 - Adds classification of `run_background_job` commands using the same safety rules as shell commands, so background jobs are now properly evaluated (and prompted or denied) based on their actual command content

docs/configuration/overview/index.md

@@ -298,10 +298,10 @@ For YAML editor autocompletion and validation, use the [Docker Agent JSON Schema
 
 ## Config Versioning
 
-Docker Agent configs are versioned. The current version is `15`. Add the version at the top of your config:
+Docker Agent configs are versioned. The current version is `16`. Add the version at the top of your config:
 
 ```yaml
-version: 15
+version: 16
 
 agents:
   root:
@@ -319,6 +319,12 @@ hint: this syntax is supported by config version 12; update the top-level 'versi
 
 Bump the `version` field as directed to enable the new syntax.
 
+Conversely, if a key was valid in an older schema version but has since been removed (for example, the `safer` shell toolset flag removed in version 15 — see the [Shell tool docs](../../tools/shell/index.md)), the hint instead tells you the field is gone and should be deleted, rather than suggesting you lower `version`:
+
+```text
+hint: 'safer' was part of config version 14 but has since been removed; delete it from your config instead of lowering the top-level 'version' field
+```
+
 ## Metadata Section
 
 Optional metadata for agent distribution via OCI registries:

docs/tools/shell/index.md

@@ -67,7 +67,7 @@ The session's [safety mode](../../configuration/permissions/index.md#safety-mode
 
 Compound shell (`a && b`, `a; b`, `a | b`) is never matched against the safe allowlist; any destructive segment falls through to ask. The full taxonomy lives in [`pkg/safety/safety_patterns.json`](https://github.com/docker/docker-agent/blob/main/pkg/safety/safety_patterns.json).
 
-See [`examples/safety_modes.yaml`](https://github.com/docker/docker-agent/blob/main/examples/safety_modes.yaml) for a full example. The legacy `safer: true` toolset flag was removed in config version 15 (it is still accepted, and ignored, by older config versions).
+See [`examples/safety_modes.yaml`](https://github.com/docker/docker-agent/blob/main/examples/safety_modes.yaml) for a full example. The legacy `safer: true` toolset flag was removed in config version 15: a config declaring version 15 or later (including a version-less config, which resolves to the latest schema) now fails to load with `unknown field "safer"` if the flag is present — delete it, it has had no effect since v1.117.0. Configs pinned to `version: "14"` or lower still accept the flag and silently ignore it.
 
 ### Sudo support
 

pkg/config/config.go

@@ -67,6 +67,8 @@ func Load(ctx context.Context, source Source, opts ...LoadOption) (*latest.Confi
 		msg := yaml.FormatError(err, true, true)
 		if hint := newerVersionHint(data, raw.Version, err); hint != "" {
 			msg += "\n" + hint
+		} else if hint := removedFieldHint(raw.Version, err); hint != "" {
+			msg += "\n" + hint
 		}
 		return nil, fmt.Errorf("parsing config file\n%s", msg)
 	}
@@ -193,16 +195,15 @@ func parseCurrentVersion(data []byte, version string) (any, error) {
 	return parser(data)
 }
 
-// newerVersionHint returns a user-facing hint when a strict-parse failure is
-// caused by a key that a newer schema version accepts. It tries the parsers
-// for every version above the declared one, in order, and points the user at
-// the smallest version that parses the config successfully. Best-effort: a
-// newer version may accept the config for unrelated reasons (laxer schema),
-// so the original unknown-field error is always shown before the hint.
 // newerVersionHint returns a hint when a parse error is caused by a key or a
 // value shape that a newer config version accepts (an unknown field, or a type
 // mismatch such as a list where an older schema only takes a string), so the
-// user is pointed at the `version` bump instead of a generic YAML error.
+// user is pointed at the `version` bump instead of a generic YAML error. It
+// tries the parsers for every version above the declared one, in order, and
+// points the user at the smallest version that parses the config successfully.
+// Best-effort: a newer version may accept the config for unrelated reasons
+// (laxer schema), so the original unknown-field error is always shown before
+// the hint.
 func newerVersionHint(data []byte, version string, parseErr error) string {
 	var unknownField *yaml.UnknownFieldError
 	var typeErr *yaml.TypeError

pkg/config/config_test.go

@@ -1197,6 +1197,31 @@ agents:
 	assert.NotContains(t, err.Error(), "hint:")
 }
 
+// TestLoadRemovedFieldHint pins the fix for issue #4175: a version-less (or
+// explicit latest-version) config with a shell toolset's `safer: true` still
+// fails — the field was intentionally removed as of config v15 (PR #4169)
+// and stays removed — but the error now points out that `safer` used to be
+// part of config version 14 and should be deleted, instead of silently
+// suggesting a `version:` downgrade the way the newer-version hint's wording
+// would.
+func TestLoadRemovedFieldHint(t *testing.T) {
+	t.Parallel()
+
+	cfgStr := `agents:
+  root:
+    model: openai/gpt-4o
+    instruction: test
+    toolsets:
+      - type: shell
+        safer: true
+`
+	_, err := Load(t.Context(), NewBytesSource("test.yaml", []byte(cfgStr)))
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), `unknown field "safer"`)
+	assert.Contains(t, err.Error(), "config version 14")
+	assert.Contains(t, err.Error(), "delete it from your config")
+}
+
 func TestLoadNewerVersionHintNumericOrdering(t *testing.T) {
 	t.Parallel()
 

pkg/config/removed_field_hint.go

@@ -0,0 +1,140 @@
+package config
+
+import (
+	"errors"
+	"reflect"
+	"regexp"
+	"slices"
+	"strconv"
+	"strings"
+
+	"github.com/goccy/go-yaml"
+)
+
+// unknownFieldNamePattern extracts the field name from a go-yaml
+// UnknownFieldError's message, which has the fixed shape `unknown field
+// "name"`.
+var unknownFieldNamePattern = regexp.MustCompile(`unknown field "([^"]*)"`)
+
+// removedFieldHint returns a user-facing hint when a strict-parse failure is
+// caused by a key that used to be part of an older, lower-numbered config
+// version's schema but is no longer part of the declared version. It
+// complements newerVersionHint: that one points forward at a version bump
+// when newer syntax is needed, this one explains that the field was
+// intentionally removed and should simply be deleted, not restored by
+// lowering the top-level 'version' field (which would just move the problem
+// to whichever version the config is eventually run against).
+func removedFieldHint(version string, parseErr error) string {
+	var unknownField *yaml.UnknownFieldError
+	if !errors.As(parseErr, &unknownField) {
+		return ""
+	}
+
+	m := unknownFieldNamePattern.FindStringSubmatch(unknownField.GetMessage())
+	if m == nil {
+		return ""
+	}
+	field := m[1]
+
+	current, err := strconv.Atoi(version)
+	if err != nil {
+		return ""
+	}
+
+	parsers, _ := versions()
+	if schemaFieldNames(parsers, version)[field] {
+		// The field exists in the declared version too, so this isn't a
+		// removed-field case (e.g. the key is just nested wrong).
+		return ""
+	}
+
+	var older []int
+	for v := range parsers {
+		if n, err := strconv.Atoi(v); err == nil && n < current {
+			older = append(older, n)
+		}
+	}
+	slices.Sort(older)
+	slices.Reverse(older)
+
+	for _, n := range older {
+		v := strconv.Itoa(n)
+		if schemaFieldNames(parsers, v)[field] {
+			return "hint: '" + field + "' was part of config version " + v +
+				" but has since been removed; delete it from your config " +
+				"instead of lowering the top-level 'version' field"
+		}
+	}
+
+	return ""
+}
+
+// schemaFieldNames returns the set of YAML keys reachable from a config
+// version's root type, discovered by reflecting over the zero value its
+// parser produces for empty input. This lets removedFieldHint answer "was
+// this key ever part of this version's schema" without hand-maintaining a
+// per-version field list that would drift from the actual Go types.
+func schemaFieldNames(parsers map[string]func([]byte) (any, error), version string) map[string]bool {
+	parser, ok := parsers[version]
+	if !ok {
+		return nil
+	}
+	zero, _ := parser(nil)
+
+	names := map[string]bool{}
+	collectFieldNames(reflect.TypeOf(zero), map[reflect.Type]bool{}, names)
+	return names
+}
+
+// collectFieldNames walks t (and, recursively, every field's type) collecting
+// the effective YAML key name for every field into out. seen guards against
+// revisiting a struct type more than once, which both saves work and avoids
+// infinite recursion on self-referential types.
+func collectFieldNames(t reflect.Type, seen map[reflect.Type]bool, out map[string]bool) {
+	for t != nil && (t.Kind() == reflect.Pointer || t.Kind() == reflect.Slice || t.Kind() == reflect.Array || t.Kind() == reflect.Map) {
+		if t.Kind() == reflect.Map {
+			collectFieldNames(t.Key(), seen, out)
+		}
+		t = t.Elem()
+	}
+	if t == nil || t.Kind() != reflect.Struct || seen[t] {
+		return
+	}
+	seen[t] = true
+
+	for f := range t.Fields() {
+		if f.PkgPath != "" && !f.Anonymous {
+			// Unexported field: invisible to every YAML/JSON decoder, so it
+			// can never be named in an UnknownFieldError.
+			continue
+		}
+		if name, ignored := yamlFieldName(f); !ignored {
+			out[name] = true
+		}
+		collectFieldNames(f.Type, seen, out)
+	}
+}
+
+// yamlFieldName returns the key name go-yaml's strict decoder matches
+// against a YAML mapping key for f, mirroring the precedence in
+// goccy/go-yaml's structField/getTag: an explicit `yaml` tag wins, then
+// `json`, and a field with neither tag falls back to its lowercased Go name
+// — untagged fields are common in this codebase (e.g. AgentConfig.Name,
+// SkillsConfig.Sources) and would otherwise be invisible to schemaFieldNames.
+// ignored is true for a field tagged "-" (yaml, or json when yaml is absent),
+// which go-yaml excludes from decoding entirely.
+func yamlFieldName(f reflect.StructField) (name string, ignored bool) {
+	tag, ok := f.Tag.Lookup("yaml")
+	if !ok {
+		tag = f.Tag.Get("json")
+	}
+	first, _, _ := strings.Cut(tag, ",")
+	switch first {
+	case "-":
+		return "", true
+	case "":
+		return strings.ToLower(f.Name), false
+	default:
+		return first, false
+	}
+}

pkg/config/removed_field_hint_test.go

@@ -0,0 +1,108 @@
+package config
+
+import (
+	"errors"
+	"testing"
+
+	"github.com/goccy/go-yaml"
+	"github.com/stretchr/testify/assert"
+
+	"github.com/docker/docker-agent/pkg/config/latest"
+)
+
+// TestRemovedFieldHint_FieldRemovedInOlderVersion pins the main case this
+// hint exists for: `safer` was part of the shell toolset through config
+// version 14 and has not existed since (it was dropped for v15, see
+// PR #4169). Declaring the latest version and hitting an unknown-field
+// error for `safer` should point at version 14, not offer to lower
+// `version` as if that were the fix.
+func TestRemovedFieldHint_FieldRemovedInOlderVersion(t *testing.T) {
+	t.Parallel()
+
+	err := &yaml.UnknownFieldError{Message: `unknown field "safer"`}
+	hint := removedFieldHint(latest.Version, err)
+
+	assert.Contains(t, hint, "'safer'")
+	assert.Contains(t, hint, "config version 14")
+	assert.Contains(t, hint, "delete it from your config")
+	assert.NotContains(t, hint, "update the top-level 'version' field", "should not read like newerVersionHint's bump-the-version advice")
+}
+
+// TestRemovedFieldHint_FieldNeverExisted ensures a key that was never valid
+// in any registered config version produces no hint: the whole point is to
+// only fire for fields we can prove were once part of the schema.
+func TestRemovedFieldHint_FieldNeverExisted(t *testing.T) {
+	t.Parallel()
+
+	err := &yaml.UnknownFieldError{Message: `unknown field "not_a_real_key_ever"`}
+	hint := removedFieldHint(latest.Version, err)
+
+	assert.Empty(t, hint)
+}
+
+// TestRemovedFieldHint_FieldStillValid guards against a false positive when
+// the "unknown field" error is really about a field being nested in the
+// wrong place rather than removed: if the declared version's own schema
+// still contains the field name somewhere, we stay silent instead of
+// claiming it was removed.
+func TestRemovedFieldHint_FieldStillValid(t *testing.T) {
+	t.Parallel()
+
+	err := &yaml.UnknownFieldError{Message: `unknown field "model"`}
+	hint := removedFieldHint(latest.Version, err)
+
+	assert.Empty(t, hint)
+}
+
+// TestRemovedFieldHint_NonUnknownFieldError ensures the hint only reacts to
+// UnknownFieldError, leaving other parse failures (type mismatches, syntax
+// errors) alone.
+func TestRemovedFieldHint_NonUnknownFieldError(t *testing.T) {
+	t.Parallel()
+
+	hint := removedFieldHint(latest.Version, errors.New("some other parse error"))
+
+	assert.Empty(t, hint)
+}
+
+// TestRemovedFieldHint_NonNumericVersion guards the strconv.Atoi bailout: a
+// malformed declared version must not panic or produce a bogus hint.
+func TestRemovedFieldHint_NonNumericVersion(t *testing.T) {
+	t.Parallel()
+
+	err := &yaml.UnknownFieldError{Message: `unknown field "safer"`}
+	hint := removedFieldHint("not-a-version", err)
+
+	assert.Empty(t, hint)
+}
+
+// TestSchemaFieldNames_SaferOnlyInV14 spot-checks the reflection helper
+// backing removedFieldHint against the known safer/instruction_file history:
+// safer is reachable from v14's Config type and not from v15 or latest;
+// instruction_file (introduced in v11) is reachable from every version at
+// or after that.
+func TestSchemaFieldNames_SaferOnlyInV14(t *testing.T) {
+	t.Parallel()
+
+	parsers, _ := versions()
+
+	assert.True(t, schemaFieldNames(parsers, "14")["safer"], "safer should still be reachable from v14's schema")
+	assert.False(t, schemaFieldNames(parsers, "15")["safer"], "safer was removed as of v15")
+	assert.False(t, schemaFieldNames(parsers, latest.Version)["safer"], "safer must not be reachable from latest")
+
+	assert.True(t, schemaFieldNames(parsers, "11")["instruction_file"])
+	assert.True(t, schemaFieldNames(parsers, latest.Version)["instruction_file"])
+}
+
+// TestSchemaFieldNames_UntaggedFieldFallsBackToLowercaseName guards the gap
+// found in review: AgentConfig.Name carries no json or yaml tag at all, so
+// go-yaml matches it against the lowercased Go field name ("name"). Without
+// replicating that fallback, schemaFieldNames would never see "name" as
+// part of the schema, and any other untagged field would be silently
+// invisible to removedFieldHint.
+func TestSchemaFieldNames_UntaggedFieldFallsBackToLowercaseName(t *testing.T) {
+	t.Parallel()
+
+	parsers, _ := versions()
+	assert.True(t, schemaFieldNames(parsers, latest.Version)["name"], "AgentConfig.Name has no json/yaml tag and must fall back to its lowercased Go name")
+}