fix(config): reject bare and null forms of unsupported sections

[shreemaan-abhishek]

Summary

Closes #33.

The previous cfg.<Section> != nil checks only rejected explicitly-empty sections (upstreams: []). YAML like upstreams: (bare key, no value) and upstreams: null unmarshalled to a nil slice and slipped past, even though the user had clearly declared an unsupported section.

Changes

• configutil.markPresentUnsupportedSections: a two-pass parse over the raw input (map[string]json.RawMessage for JSON, map[string]yaml.Node for YAML) that flips the four unsupported section fields (upstreams / consumer_groups / plugin_configs / service_templates) from nil to a non-nil empty slice when the key is present, regardless of value shape.
• Route config validate through configutil.ReadConfigFile so it inherits the same key-presence detection that diff/sync already use. Drops validate's now-duplicate file-reading code (bytes / encoding/json / os / gopkg.in/yaml.v3 imports go with it).
• Tests: TestConfigValidate_EmptyUnsupportedSections is extended with 14 subtests covering empty-list / bare / null shapes for all four sections, plus JSON-null variants. All pass:

  --- PASS: ../upstreams_empty_list, /upstreams_bare, /upstreams_null
  --- PASS: ../consumer_groups_empty_list, /_bare, /_null
  --- PASS: ../plugin_configs_empty_list, /_bare, /_null
  --- PASS: ../service_templates_empty_list, /_bare, /_null
  --- PASS: ../json_upstreams_null, /json_consumer_groups_null
  

Test plan

• go build, go vet, full go test ./... green locally.
• 14 new subtests in TestConfigValidate_EmptyUnsupportedSections all pass.

Why this isn't merged with PR #43

Kept scope-tight on PR #43 (json.RawMessage exporter audit + CI verbosity). This change is config-validation hardening, a different concern with its own test surface.

Closes #33. Part of #22.

Summary by CodeRabbit

Bug Fixes

• Configuration file validation now consistently handles empty or improperly formatted unsupported resource sections regardless of input format (empty lists, bare keys, null values).
• Enhanced validation ensures bare keys and explicit null values for unsupported sections are properly normalized and rejected during validation.

[coderabbitai]

📝 Walkthrough

Walkthrough

The PR extends config validation to detect bare keys and null values for unsupported top-level sections. ReadConfigFile adds a key-presence normalization pass that converts nil slices to empty slices when keys are present, enabling typed nil checks to catch all empty forms. Validation refactors to use this shared normalization instead of local parsing. Test coverage expands to verify bare keys, nulls, and explicit empty lists are all rejected.

Changes

Unsupported section bare/null key-presence detection

Layer / File(s)	Summary
Key-presence normalization in ReadConfigFile pkg/cmd/config/configutil/configutil.go	ReadConfigFile calls markPresentUnsupportedSections after unmarshalling to detect presence of unsupported top-level keys (upstreams, consumer_groups, plugin_configs, service_templates) in raw JSON/YAML input and normalize nil slices to non-nil empty slices, ensuring typed nil validation rejects bare keys and null values consistently. ValidateSupportedSections comments updated to reflect this normalization behavior.
Validation consolidation onto shared normalization pkg/cmd/config/validate/validate.go	validateRun refactors to invoke configutil.ReadConfigFile for config parsing and file I/O, eliminating local JSON/YAML unmarshalling and the readFile helper. Import set updated to depend on configutil. ValidateConfigFile comment revised to document that normalization is now handled by ReadConfigFile.
Test coverage for all unsupported section empty forms pkg/cmd/config/validate/validate_test.go	TestConfigValidate_EmptyUnsupportedSections expanded to enumerate YAML bare keys (upstreams:), null values (upstreams: null), JSON nulls, and explicit empty lists (upstreams: []) across unsupported sections, verifying all three empty representations are rejected with consistent error messages.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 6

✅ Passed checks (6 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: fixing config validation to reject bare and null forms of unsupported sections, which is the core objective of the PR.
Linked Issues check	✅ Passed	All requirements from issue #33 are met: two-pass parse detects key presence in markPresentUnsupportedSections, validation rejects bare/null forms, and tests cover all required cases including bare keys and null values for all four unsupported sections.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to issue #33: key-presence detection logic, routing config validate through ReadConfigFile, removing duplicate file-reading code, and extending validation tests—no extraneous modifications detected.
E2e Test Quality Review	✅ Passed	Comprehensive E2E test with 14 subtests covering all unsupported sections across all value forms. Proper error handling, no race conditions, directly solves issue #33 with no unrelated changes.
Security Check	✅ Passed	Config validation improvements with no sensitive data exposure, unencrypted secrets, auth bypass, cross-resource issues, TLS errors, resource conflicts, or secret reference resolution vulnerabilities.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch config-validate-reject-bare-null-sections

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

pkg/cmd/config/validate/validate_test.go (1)

375-376: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

JSON-null subtests are currently written as .yaml, so they may miss the JSON code path.

json_* cases should use a .json filename; otherwise they can still parse as YAML and give false confidence for JSON-specific normalization.

Suggested fix

 		t.Run(tc.name, func(t *testing.T) {
 			ios, _, _, _ := iostreams.Test()
-			filePath := filepath.Join(t.TempDir(), "config.yaml")
+			ext := ".yaml"
+			if strings.HasPrefix(tc.name, "json_") {
+				ext = ".json"
+			}
+			filePath := filepath.Join(t.TempDir(), "config"+ext)
 			require.NoError(t, os.WriteFile(filePath, []byte(tc.body), 0o644))

 import (
 	"net/http"
 	"os"
 	"path/filepath"
+	"strings"
 	"testing"

As per coding guidelines, **/*.go: Write tests for every code change.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/cmd/config/validate/validate_test.go` around lines 375 - 376, The
JSON-specific subtests (named like "json_*") are writing test fixtures to a
.yaml path which allows YAML parsing to succeed and skips the JSON code path;
update the test so when tc.name or the test case indicates JSON (e.g., json_*
cases) you create the temp file with a .json extension instead of .yaml by
adjusting filePath (the filepath.Join(t.TempDir(), "...") call) so it uses
".json" for JSON cases, ensuring the test writes tc.body to that .json file and
exercises the JSON parsing/normalization path.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@pkg/cmd/config/validate/validate_test.go`:
- Around line 375-376: The JSON-specific subtests (named like "json_*") are
writing test fixtures to a .yaml path which allows YAML parsing to succeed and
skips the JSON code path; update the test so when tc.name or the test case
indicates JSON (e.g., json_* cases) you create the temp file with a .json
extension instead of .yaml by adjusting filePath (the filepath.Join(t.TempDir(),
"...") call) so it uses ".json" for JSON cases, ensuring the test writes tc.body
to that .json file and exercises the JSON parsing/normalization path.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: d149dcd0-07f7-417a-89a2-6e7686e8ddef

📥 Commits

Reviewing files that changed from the base of the PR and between 3fbbb89 and 1b57c78.

📒 Files selected for processing (3)

• pkg/cmd/config/configutil/configutil.go
• pkg/cmd/config/validate/validate.go
• pkg/cmd/config/validate/validate_test.go

[Copilot]

Pull request overview

This PR hardens declarative config validation so that key presence for unsupported top-level sections is detected consistently across YAML and JSON, including “bare key” (upstreams:) and explicit null forms that previously unmarshalled to nil and slipped past validation. It also aligns config validate with the existing configutil.ReadConfigFile parsing path used by other config commands.

Changes:

• Add a raw key-presence pass in configutil.ReadConfigFile to normalize declared-but-empty unsupported sections into non-nil empty slices.
• Route config validate through configutil.ReadConfigFile (removing duplicate file reading / JSON-vs-YAML parsing code).
• Expand validation tests to cover [], bare, and null shapes (plus JSON null cases) for unsupported sections.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File	Description
pkg/cmd/config/validate/validate.go	Switches validate to use configutil.ReadConfigFile, ensuring bare/null unsupported sections are detected.
pkg/cmd/config/validate/validate_test.go	Extends TestConfigValidate_EmptyUnsupportedSections with additional YAML/JSON cases for bare and null forms.
pkg/cmd/config/configutil/configutil.go	Adds markPresentUnsupportedSections two-pass parsing to treat declared unsupported sections as present even when unmarshalling yields nil.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.