Add profiles to x/config/cli by nolag · Pull Request #2456 · smartcontractkit/chainlink-common

nolag · 2026-10-09T16:25:36Z

No description provided.

github-actions · 2026-10-09T16:27:57Z

📊 API Diff Results

No changes detected for module github.com/smartcontractkit/chainlink-common

View full report

Copilot

🟡 Changes recommended

Profile help can panic when a profile sets a pointer subsection to nil.

1 open finding

Avoid panic when rendering help for nil profile subsections · New

What changed in this PR

Adds profile-based defaults to the config CLI, including nested-section support, source precedence, help output, tests, and examples.

Changes:

Introduces Profile and ProfileWithSelector.
Tracks source-supplied fields so they override profiles.
Adds profile examples, documentation, and tests.

File	Description
`x/config/go.mod`	Adds deep-cloning dependency.
`x/config/go.sum`	Updates dependency checksums.
`x/config/cli/register.go`	Records required-field metadata.
`x/config/cli/README.md`	Documents profile APIs.
`x/config/cli/profile.go`	Implements profile selection, application, and help.
`x/config/cli/profile_test.go`	Tests profile behavior.
`x/config/cli/options.go`	Updates option documentation.
`x/config/cli/entry.go`	Applies profiles after decoding sources.
`x/config/cli/entries.go`	Reports source-set map fields.
`x/config/cli/binder.go`	Documents profile processing order.
`x/config/cli/examples/profiles/README.md`	Documents standalone profiles.
`x/config/cli/examples/profiles/main.go`	Adds standalone example.
`x/config/cli/examples/profiles/example.toml`	Adds example configuration.
`x/config/cli/examples/profiles/appconfig/gen/main.go`	Adds documentation generator.
`x/config/cli/examples/profiles/appconfig/doccomments_gen.go`	Adds generated field documentation.
`x/config/cli/examples/profiles/appconfig/config.go`	Defines example chain configuration.
`x/config/cli/examples/nested_profiles/README.md`	Documents nested profiles.
`x/config/cli/examples/nested_profiles/main.go`	Adds nested profile example.
`x/config/cli/examples/nested_profiles/example.toml`	Adds nested example configuration.
`x/config/cli/examples/nested_profiles/appconfig/gen/main.go`	Adds nested documentation generator.
`x/config/cli/examples/nested_profiles/appconfig/doccomments_gen.go`	Adds generated nested documentation.
`x/config/cli/examples/nested_profiles/appconfig/config.go`	Defines nested example configuration.

Files not reviewed (2)

x/config/cli/examples/nested_profiles/appconfig/doccomments_gen.go: Generated file
x/config/cli/examples/profiles/appconfig/doccomments_gen.go: Generated file

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Copilot

🟡 Changes recommended

Profile application currently breaks per-key map precedence, retains stale profiles across executions, and accepts invalid selector types.

4 open findings

Overlay only supplied dynamic map entries · New
Reject pointer-based profile selector key types · New
Require a flag-bound profile selector · New
Restore defaults when selecting an unknown profile · New

1 resolved since last review

Avoid panic when rendering help for nil profile subsections

Files not reviewed (2)

x/config/cli/examples/nested_profiles/appconfig/doccomments_gen.go: Generated file
x/config/cli/examples/profiles/appconfig/doccomments_gen.go: Generated file

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Map-entry environment variables can fail to override profile-provided keys because their names are resolved before profiles are applied.

2 open findings

4 resolved since last review

Files not reviewed (2)

x/config/cli/examples/nested_profiles/appconfig/doccomments_gen.go: Generated file
x/config/cli/examples/profiles/appconfig/doccomments_gen.go: Generated file

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Profile-key validation and help rendering have unresolved correctness issues.

4 open findings

Reject non-reflexive profile keys such as NaN · New
Detect case collisions in nested profile maps · New
Use EqualFold for profile key collision checks · New
Format list and map values in profile help · New

2 resolved since last review

Files not reviewed (2)

x/config/cli/examples/nested_profiles/appconfig/doccomments_gen.go: Generated file
x/config/cli/examples/profiles/appconfig/doccomments_gen.go: Generated file

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Profile setup can fail on cyclic defaults or nil profile functions, and help can omit effective required defaults.

3 open findings

4 resolved since last review

Files not reviewed (2)

x/config/cli/examples/nested_profiles/appconfig/doccomments_gen.go: Generated file
x/config/cli/examples/profiles/appconfig/doccomments_gen.go: Generated file

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Map-entry environment resolution can incorrectly fail against stale defaults before the selected profile is applied.

2 open findings

3 resolved since last review

Files not reviewed (2)

x/config/cli/examples/nested_profiles/appconfig/doccomments_gen.go: Generated file
x/config/cli/examples/profiles/appconfig/doccomments_gen.go: Generated file

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

Repeated execution can reuse a stale selector after its source is removed, causing the wrong profile to be applied.

1 open finding

Omitted selector reuses stale value from mutated destination · New

2 resolved since last review

Files not reviewed (2)

x/config/cli/examples/nested_profiles/appconfig/doccomments_gen.go: Generated file
x/config/cli/examples/profiles/appconfig/doccomments_gen.go: Generated file

🧠 Review effort: Balanced

Copilot

🟡 Changes recommended

The caller-owned profile map can be mutated after validation, bypassing safeguards and potentially causing runtime panics.

1 open finding

Clone profile map before validation to prevent post-registration mutation · New

1 resolved since last review

Omitted selector reuses stale value from mutated destination

Files not reviewed (2)

x/config/cli/examples/nested_profiles/appconfig/doccomments_gen.go: Generated file
x/config/cli/examples/profiles/appconfig/doccomments_gen.go: Generated file

🧠 Review effort: Balanced

Copilot

🔵 Needs a closer look

Reflection-based section reconstruction and multi-source map layering warrant final human review despite broad test coverage.

2 open findings

Incorrect comma at sentence end · New
Incorrect callback type in public API documentation · New

1 resolved since last review

Clone profile map before validation to prevent post-registration mutation

Files not reviewed (2)

x/config/cli/examples/nested_profiles/appconfig/doccomments_gen.go: Generated file
x/config/cli/examples/profiles/appconfig/doccomments_gen.go: Generated file

🧠 Review effort: Balanced

jmank88 · 2026-10-14T16:37:34Z

+
+// visitEntries calls visit for m and each map below it on leaf's path, then for each value leaf sets, named as their
+// flags would be, such as section.chains.mainnet.labels.
+func visitEntries(leaf *entryLeaf, name string, m reflect.Value, depth int, visit func(name string, v reflect.Value, isMap bool) error) error {


Did you consider writing this as an iterator, returning Seq2[string,reflect.Value]? The unused/suppressed error handling goes away. Although maybe the third isMap param throws things off by forcing some kind of wrapper struct 🤷

jmank88 · 2026-10-14T16:46:27Z

+	return len(k.goPath) > len(scope) && slices.Equal(k.goPath[:len(scope)], scope)
+}
+
+func scopesOverlap(a, b []string) bool {


This implementation reads a little stricter to me than I would expect from "overlap". Is there a way to say that one prefixes the other? Or points to an ancestor node? Is a "scope" different from a path?

jmank88 · 2026-10-14T16:48:41Z

+	for _, k := range entry.leaves {
+		parent := lookupField(v, k.goPath[:len(k.goPath)-1])
+		// Types are checked as a struct shares its first field's address.
+		if parent.Type() != reflect.TypeFor[S]() || parent.Addr().Pointer() != wantSection {


Does it matter if we construct just one instance of reflect.TypeFor[S]() outside the for loop?

jmank88 · 2026-10-14T16:51:54Z

+	}
+
+	defer func() {
+		if r := recover(); r != nil {


Is it inevitable that we have to recover generally? (i.e. can users of the lib cause this to happen, so no way around it?)
Or are we only worried about our own internal deliberate panics, meaning we could consider checking more narrowly for a specific error type?

jmank88 · 2026-10-14T16:55:03Z

+		for _, other := range texts[i+1:] {
+			if strings.EqualFold(text, other) {
+				return fmt.Errorf("profile %v gives %s the keys %q and %q, which differ only in case, so an env var for either is ambiguous", profile, key, text, other)
+			}
+		}


Could shed some indentation here with package slices funcs but not sure it will be much cleaner since you'll need the index to reproduce the error message.

Oh but more generally can the O(n^2) loop be avoided? e.g. by using https://pkg.go.dev/golang.org/x/text/cases#Fold to do just two passes?

Done (will open another PR) but not with cases.Fold.
Claude says:

Fold does full folding: joins ß and ss. Runtime env matching uses strings.EqualFold, which keeps them apart, so Fold would reject keys runtime finds fine. Instead foldText maps each rune to smallest rune in its unicode.SimpleFold orbit. Same equality as EqualFold, one map lookup per key.

jmank88 · 2026-10-14T17:02:35Z

+		}
+
+		built := clone.Slowly(defaults).(S)
+		vBuilt := reflect.ValueOf(&built).Elem()


Is this shorter form equivalent?

Suggested change

vBuilt := reflect.ValueOf(&built).Elem()

vBuilt := reflect.ValueOf(built)

reflect.ValueOf(built) not addressable: next line's allocateField(...).Set panics "unaddressable value". &built + .Elem() needed.

nolag requested a balanced review from Copilot October 9, 2026 16:25

Copilot started reviewing on behalf of nolag October 9, 2026 16:26

View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

Comment thread x/config/cli/profile.go Outdated

nolag force-pushed the rtinianov_cli_profiles branch from 17d0e01 to 9404cdc Compare October 9, 2026 16:42

nolag requested a balanced review from Copilot October 9, 2026 16:42

Copilot started reviewing on behalf of nolag October 9, 2026 16:43

View session

Copilot AI reviewed Oct 9, 2026

View reviewed changes

Comment thread x/config/cli/profile.go Outdated

Comment thread x/config/cli/profile.go

Comment thread x/config/cli/profile.go Outdated

Comment thread x/config/cli/profile.go Outdated

nolag force-pushed the rtinianov_cli_profiles branch 2 times, most recently from 2a6849a to da8ed1e Compare October 13, 2026 13:02

nolag requested a balanced review from Copilot October 13, 2026 13:04

Copilot started reviewing on behalf of nolag October 13, 2026 13:05

View session