Skip to content

Commit ba407fc

Browse files
committed
fix: preserve dual-schema-source guard through layer translation
Copilot review (PR #288): when a single config layer sets both a non-empty `schema:` and a non-empty `inline-schema:`, extractSchemaSourceFromSettings returned at the `schema` arm and TranslateLayerSettings then stripped both keys, silently dropping the inline source. The rule's rejectDualSchemaSettings guard in ApplySettings was bypassed because translation removed the keys before ApplySettings ran, and top-level cfg.Rules / overrides / convention presets are not covered by validateKindSchemaSources. TranslateLayerSettings now detects a dual-source layer (hasDualSchemaSource, mirroring rejectDualSchemaSettings' non-empty semantics) and passes the layer through untouched, so the keys survive deep-merge and the existing guard still surfaces the original "cannot set both" config error. Cross-layer composition is unaffected — the check only fires when one map carries both. Added regression tests; new code is 100% line and branch covered.
1 parent 38a0b69 commit ba407fc

2 files changed

Lines changed: 63 additions & 0 deletions

File tree

internal/rules/requiredstructure/inline_schema_test.go

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -456,6 +456,48 @@ func TestApplySettings_RejectsBothSources(t *testing.T) {
456456
assert.Contains(t, err.Error(), "cannot set both")
457457
}
458458

459+
// TestTranslateLayerSettings_DualSourcePreservesGuard regresses
460+
// the Copilot review on PR #288: when one config layer sets both
461+
// a non-empty `schema:` and a non-empty `inline-schema:`, the
462+
// translator must NOT strip the keys (which would silently drop
463+
// the inline source). It passes the layer through unchanged so
464+
// the keys survive deep-merge and ApplySettings' dual-source
465+
// guard still surfaces the original config error.
466+
func TestTranslateLayerSettings_DualSourcePreservesGuard(t *testing.T) {
467+
r := &Rule{}
468+
dual := map[string]any{
469+
"schema": "schemas/rfc.md",
470+
"inline-schema": map[string]any{
471+
"sections": []any{map[string]any{"heading": "X"}},
472+
},
473+
}
474+
out := r.TranslateLayerSettings(dual)
475+
// Untouched: both legacy keys survive, no schema-sources added.
476+
assert.Equal(t, "schemas/rfc.md", out["schema"],
477+
"dual-source layer must pass through with `schema` intact")
478+
assert.Contains(t, out, "inline-schema",
479+
"dual-source layer must pass through with `inline-schema` intact")
480+
assert.NotContains(t, out, "schema-sources",
481+
"dual-source layer must not be translated to schema-sources")
482+
// The surviving keys still trip the rule's guard.
483+
err := r.ApplySettings(out)
484+
require.Error(t, err)
485+
assert.Contains(t, err.Error(), "cannot set both")
486+
}
487+
488+
// TestTranslateLayerSettings_SingleSourcesStillTranslate confirms
489+
// the dual-source guard does not regress the normal single-source
490+
// translation (only one of the two keys, non-empty).
491+
func TestTranslateLayerSettings_SingleSourcesStillTranslate(t *testing.T) {
492+
r := &Rule{}
493+
out := r.TranslateLayerSettings(map[string]any{"schema": "x.md"})
494+
srcs, ok := out["schema-sources"].([]any)
495+
require.True(t, ok)
496+
require.Len(t, srcs, 1)
497+
assert.Equal(t, "x.md", srcs[0].(map[string]any)["file"])
498+
assert.NotContains(t, out, "schema")
499+
}
500+
459501
func TestApplySettings_AllowsEmptySchemaWithInline(t *testing.T) {
460502
// An empty `schema:""` next to a real `inline-schema` is the
461503
// merge-clears-prior-state state; the rule must still accept it.

internal/rules/requiredstructure/rule.go

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -347,6 +347,16 @@ func (r *Rule) SettingMergeMode(key string) rule.MergeMode {
347347
// input map is treated as read-only; a new map is returned only
348348
// when a legacy key is present.
349349
func (r *Rule) TranslateLayerSettings(settings map[string]any) map[string]any {
350+
// A single layer that sets BOTH a non-empty `schema:` and a
351+
// non-empty `inline-schema:` is a config error. Pass the layer
352+
// through untouched so the keys survive deep-merge and the
353+
// rule's own rejectDualSchemaSettings (run from ApplySettings)
354+
// still surfaces the original error — stripping them here would
355+
// silently drop the inline source. Cross-layer composition is
356+
// unaffected: this only fires when one map carries both.
357+
if hasDualSchemaSource(settings) {
358+
return settings
359+
}
350360
source, hadKey := extractSchemaSourceFromSettings(settings)
351361
if !hadKey {
352362
return settings
@@ -361,6 +371,17 @@ func (r *Rule) TranslateLayerSettings(settings map[string]any) map[string]any {
361371
return out
362372
}
363373

374+
// hasDualSchemaSource reports whether one settings map sets both a
375+
// non-empty `schema:` path and a non-empty `inline-schema:` map.
376+
// It mirrors rejectDualSchemaSettings' non-empty semantics so the
377+
// translator and the rule's guard agree on what counts as a
378+
// dual-source layer.
379+
func hasDualSchemaSource(s map[string]any) bool {
380+
path, _ := s["schema"].(string)
381+
inline, _ := s["inline-schema"].(map[string]any)
382+
return path != "" && len(inline) > 0
383+
}
384+
364385
// extractSchemaSourceFromSettings inspects a settings map for a
365386
// schema-source declaration. It returns (source, true) when either
366387
// legacy key is present — even if the value is empty / no-op — so

0 commit comments

Comments
 (0)