update-feature-config
GitHub指导在 authgear.features.yaml 中添加或修改特性配置字段。强调必须实现字段级合并逻辑以防止配置覆盖,并规定需编写测试用例验证多层配置的独立保留。
Trigger Scenarios
Install
npx skills add authgear/authgear-server --skill update-feature-config -g -y
SKILL.md
Frontmatter
{
"name": "update-feature-config",
"description": "Add or change a feature config field\/section in pkg\/lib\/config (authgear.features.yaml). Use when adding a new feature config field, adding a new top-level section, or touching an existing section's Merge implementation."
}
Update Feature Config
Feature config (authgear.features.yaml) is merged across layers — code
default ← cluster ← plan ← app override — via FeatureConfig.Merge
(pkg/lib/config/feature.go), which reflects over every top-level field and
dispatches to that section's own Merge in pkg/lib/config/feature_*.go.
Hard requirement: merge must be field-level, never whole-section replace
Historically, most sections implemented Merge as a wholesale swap:
// WRONG for any section with more than one leaf field
func (c *XFeatureConfig) Merge(layer *FeatureConfig) MergeableFeatureConfig {
if layer.X == nil {
return c
}
return layer.X
}
This is a real, reachable bug class, not a theoretical one: if a lower layer
(e.g. a plan) sets field A and a higher layer (e.g. an app override) later
sets a sibling field B — without repeating A — the whole-section swap
silently resets A back to its code default. Plan/app documents are
routinely partial, hand-authored YAML, so this triggers in practice, not just
in edge cases. It was found and fixed across identity, authentication,
authenticator, ui, hook, collaborator, messaging.rate_limits, and
test_mode — do not reintroduce it in a new section or a new field on an
existing section.
Every new field on an existing multi-field section, and every new top-level section, must merge field-level:
- Follow the reference pattern in
OAuthClientFeatureConfig.Merge(pkg/lib/config/feature_oauth.go): nil-safe guards first (if c == nil && layer == nil { return nil },if c == nil { return layer },if layer == nil { return c }), then per-fieldif layer.X != nil { c.X = layer.X }for every field. - If a section/sub-object genuinely has only one field, a whole-object
replace at that level is fine — there's nothing else to lose. But if that
one field is itself an object with siblings further down, cascade the
field-level merge all the way down to where the real siblings are, even
through single-field wrapper levels. See
feature_authenticator.go'sAuthenticator → Password → Policycascade:AuthenticatorandPasswordeach have only one field, butPolicyhas three siblings that must merge independently — so the cascade goes three levels deep, not stopping at the first single-field level. - Never write
if layer.Section == nil { return c }; return layer.Sectionfor a section with more than one leaf field, directly or transitively.
Required test coverage
Every new/changed Merge implementation needs a case in
pkg/lib/config/testdata/merge_feature.yaml: one layer sets field A only,
a later layer sets a sibling field B only (never repeating A) — assert
the final effective config has both A (from the first layer) and B
(from the second), not A reset to its default. Pick values that are not
the code default for the field being tested — otherwise a whole-section
regression would silently produce the "right" value by accident and the test
wouldn't catch it. See the existing hook/collaborator/identity cases in
that file for the pattern.
Schema/runtime consistency
Before adding a JSON schema constraint on a feature config field
(minItems, minLength, enum, required, etc. in the
FeatureConfigSchema.Add(...) block), check what the field's actual
consumer code does with edge-case values (nil, empty, zero) — grep for
where the field is read at runtime. A constraint that's stricter than the
runtime semantics can silently make a legitimately meaningful value
unreachable. Concrete case: PhoneInputFeatureConfig.allowlist had
"minItems": 1, but IntersectAllowlist (pkg/lib/config/utils.go) already
treated an empty allowlist as "no restriction" — the schema blocked the one
input (allowlist: []) that would have cleanly expressed "clear this
override," forcing an awkward, undiscoverable workaround
(phone_input: {} with the field omitted) instead. Don't add a schema
constraint "for safety" without confirming the runtime already needs it.
Don't tag a scalar field omitempty if its zero value is a real default
SetFieldDefaults (pkg/lib/config/default.go) already makes every section
pointer non-nil via its generic reflection walk, regardless of whether that
section implements SetDefaults() — a section is never actually "absent" in
a parsed/defaulted FeatureConfig. If a plain (non-pointer) bool/string/
int/float field's zero value (false/""/0) is that field's real,
intended default — not a stand-in for "not set" — tagging it omitempty
doesn't skip anything meaningful when parsing, but it does hide that value
from JSON output: encoding/json treats the zero value as "empty" and
omits the key, so a fully-resolved section marshals as {} instead of e.g.
{"disabled": false}. This makes the Site Admin API's
effective_plan_feature_config/effective_app_feature_config (and any
other JSON consumer of FeatureConfig) show a resolved section as if it
were empty/unset. Fix: drop omitempty — plain json:"disabled". This is
always safe, because Merge() for these fields already operates on the
section's pointer-nil-ness (see the field-level merge rule above), never
the leaf scalar's zero value — removing omitempty never changes merge or
validation behavior, only what the field looks like once marshaled.
This is a different situation from the slice case in "Schema/runtime
consistency" above (PhoneInputFeatureConfig.allowlist): there, nil and an
explicit empty slice are two different meaningful values (inherit vs.
explicitly cleared), so the fix was omitzero (which only omits the true
zero value, nil), not simply dropping the tag. A plain scalar only has one
value to begin with, so just remove omitempty entirely — don't reach for
omitzero there, it would be a no-op.
Test with a real marshal, not ShouldResemble on parsed structs. Every
existing test in this package compares parsed Go structs, which can't tell
omitempty apart from no tag at all — that's exactly why this went
unnoticed for nine fields across five sections. Marshal with
encoding/json.Marshal (or sigs.k8s.io/yaml.Marshal, which calls it
internally — this is what viewEffectiveResource's merge fold does) and
assert on the resulting shape, e.g.
TestFeatureConfigDisabledFieldsSerializeExplicitly in feature_test.go.
When auditing for this, grep the whole package by field, not file by
file. A file having a SetDefaults() for one field doesn't mean every
field in that file is covered — feature_identity.go has one for
BiometricFeatureConfig (a pointer-scalar field) while
LoginIDPhoneFeatureConfig.Disabled and
OAuthSSOProviderFeatureConfig.Disabled (plain-bool, single-field sections
in that same file) still had the bug. Use:
grep -nE '^\s*[A-Z][A-Za-z0-9_]*\s+(bool|string|int|int32|int64|float32|float64)\s+`json:"[^"]*,omitempty"`' pkg/lib/config/feature_*.go
References
pkg/lib/config/feature.go— top-levelFeatureConfig.Mergedispatcherpkg/lib/config/feature_*.go— per-sectionMergeimplementationspkg/lib/config/testdata/merge_feature.yaml— shared merge test fixturepkg/lib/config/testdata/parse_feature_tests.yaml— schema validation test fixturepkg/lib/config/default.go—SetFieldDefaults, the generic reflection walkpkg/lib/config/feature_test.go'sTestFeatureConfigDisabledFieldsSerializeExplicitly— marshal-based test pattern for the omitempty rule above
Version History
- e619570 Current 2026-08-16 07:31


