diff --git a/CHANGELOG.md b/CHANGELOG.md index 026d79ea1..221296c31 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/docs/configuration/overview/index.md b/docs/configuration/overview/index.md index 73497f4b3..f0731dd0c 100644 --- a/docs/configuration/overview/index.md +++ b/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: diff --git a/docs/tools/shell/index.md b/docs/tools/shell/index.md index 6ec47eb5d..b00001fb4 100644 --- a/docs/tools/shell/index.md +++ b/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 diff --git a/pkg/config/config.go b/pkg/config/config.go index 7110f0c68..b3d076110 100644 --- a/pkg/config/config.go +++ b/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 diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index 0dffa988a..c1ad29819 100644 --- a/pkg/config/config_test.go +++ b/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() diff --git a/pkg/config/removed_field_hint.go b/pkg/config/removed_field_hint.go new file mode 100644 index 000000000..d293c0a7b --- /dev/null +++ b/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 + } +} diff --git a/pkg/config/removed_field_hint_test.go b/pkg/config/removed_field_hint_test.go new file mode 100644 index 000000000..f30f18354 --- /dev/null +++ b/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") +}