Conversation
…o detection Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
Co-authored-by: lpcox <15877973+lpcox@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR improves TOML configuration diagnostics in the gateway by adding precise parse error locations (line/column), warning on unknown/typoed keys, and switching to streaming TOML decoding for better memory behavior.
Changes:
- Parse TOML via
toml.NewDecoderand wraptoml.ParseErrorwith line/column details. - Detect unknown TOML keys via
MetaData.Undecoded()and emit warnings. - Add tests and update documentation/examples describing the new behavior.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/config/config_core.go | Switches to streaming TOML decode; adds line/column parse errors and unknown-key warnings via logger. |
| internal/config/config_test.go | Adds tests covering parse errors with column info, unknown key scenarios, large config parsing, and duplicate keys. |
| config.example.toml | Updates example comments to describe unknown-key warnings and line/column parse errors. |
| README.md | Adds a new section explaining config validation/error handling with examples. |
Comments suppressed due to low confidence (3)
internal/config/config_test.go:1192
TestLoadFromFile_MultipleUnknownKeyssimilarly doesn't assert that multiple unknown keys are actually reported; it only checks that misspelled fields fell back to defaults. To ensure the behavior described in the test name, capture the warning output and assert it includes each expected unknown key (e.g., gateway.startup_timout, gateway.tool_timout, servers.test.typ).
// Enable debug logging to capture warnings
SetDebug(true)
defer SetDebug(false)
// Should still load successfully
cfg, err := LoadFromFile(tmpFile)
require.NoError(t, err, "LoadFromFile() should succeed even with multiple unknown keys")
require.NotNil(t, cfg, "Config should not be nil")
// Correctly spelled fields should work
assert.Equal(t, 8080, cfg.Gateway.Port, "Port should be set correctly")
// Misspelled fields should use defaults
assert.Equal(t, DefaultStartupTimeout, cfg.Gateway.StartupTimeout, "StartupTimeout should be default")
assert.Equal(t, DefaultToolTimeout, cfg.Gateway.ToolTimeout, "ToolTimeout should be default")
internal/config/config_test.go:1245
- In
TestLoadFromFile_InvalidTOMLDuplicateKey, the comment says the error should mention the duplicate key, but the assertion only checks for "line". This makes the test less meaningful and could miss regressions where a different parse error happens. Consider asserting the error mentions the duplicated key name (e.g., "port") or a duplicate/"already defined" message in addition to the position info.
// Error should mention the duplicate key
assert.Contains(t, err.Error(), "line", "Error should mention line number")
}
internal/config/config_test.go:1122
TestLoadFromFile_ParseErrorWithColumnNumberallows passing when the error only contains "line 2" but not "column". Since the test name/PR intent is to ensure column reporting, this should assert the presence of the "column" token (or a regex likeline \d+, column \d+) so it actually fails if column info regresses.
// Error should contain line and column information from our improved error format
errMsg := err.Error()
assert.Contains(t, errMsg, "line", "Error should mention line number")
// Our improved format includes "column" explicitly when ParseError is detected
assert.True(t, strings.Contains(errMsg, "column") || strings.Contains(errMsg, "line 2"),
"Error should mention column or line position, got: %s", errMsg)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Enable debug logging to capture warning about unknown key | ||
| SetDebug(true) | ||
| defer SetDebug(false) | ||
|
|
||
| // Should still load successfully, but warning will be logged | ||
| cfg, err := LoadFromFile(tmpFile) | ||
| require.NoError(t, err, "LoadFromFile() should succeed even with unknown keys") | ||
| require.NotNil(t, cfg, "Config should not be nil") | ||
|
|
||
| // Port should be default since "prot" was not recognized | ||
| assert.Equal(t, DefaultPort, cfg.Gateway.Port, "Port should be default since 'prot' is unknown") | ||
| assert.Equal(t, "test-key", cfg.Gateway.APIKey, "API key should be set correctly") |
There was a problem hiding this comment.
TestLoadFromFile_UnknownKeysInGateway claims to test unknown-key detection, but it never asserts that a warning was emitted (it only checks defaults). This can pass even if the warning/logging path regresses. Consider capturing stderr output from SetDebug(true) (or initializing the file logger in-test) and asserting the log contains the expected "Unknown configuration key" message and the specific key name.
This issue also appears in the following locations of the same file:
- line 1179
- line 1243
- line 1117
| return nil, fmt.Errorf("failed to parse TOML at line %d, column %d: %s", | ||
| perr.Position.Line, perr.Position.Col, perr.Message) | ||
| } | ||
| // Try value type (used by toml.Decode) |
There was a problem hiding this comment.
The comment "Try value type (used by toml.Decode)" is misleading now that this function uses toml.NewDecoder(file).Decode. If the value-type assertion is kept for compatibility, consider updating the comment to describe when/why toml.ParseError might be returned as a value in this code path.
| // Try value type (used by toml.Decode) | |
| // Also handle value-type ParseError for compatibility with older or alternate error paths |
Configuration parse errors lacked position context and silently ignored typos in field names, leading to debugging friction and silent misconfigurations.
Changes
Precise error locations
Extract line and column from
ParseError.Positionfor pinpoint debugging:Unknown key detection
Use
MetaData.Undecoded()to catch typos and deprecated fields:Generates warnings:
Streaming decoder
Replace
os.ReadFile+toml.Decodewithtoml.NewDecoderfor memory efficiency with large config files.Test coverage
Documentation
Added "Configuration Validation and Error Handling" section to README with examples and best practices.
Warning
Firewall rules blocked me from connecting to one or more addresses (expand for details)
I tried to connect to the following addresses, but was blocked by firewall rules:
example.com/tmp/go-build4210791917/b271/launcher.test /tmp/go-build4210791917/b271/launcher.test -test.testlogfile=/tmp/go-build4210791917/b271/testlog.txt -test.paniconexit0 -test.timeout=10m0s -test.v=true 64/src/net -trimpath 64/pkg/tool/linux_amd64/vet -p github.com/davec--norc -lang=go1.16 64/pkg/tool/linux_amd64/vet -I 519236/b216/_pkg_.a cfg fig.test --gdwarf-5 b/gh-aw-mcpg/intdocker-cli-plugin-metadata -o fig.test(dns block)Co-autho--format=format:%H %ct %D ache/go/1.25.6/x64/pkg/tool/linux_amd64/vet ache/go/1.25.6/x/opt/hostedtoolcache/go/1.25.6/x64/pkg/tool/linux_amd64/compile -I` (dns block)
Co-autho-buildid=HBV11mEKIlvfULJyOluM/u0/tmp/go-build673569813/b262/vet.cfg ache/go/1.25.6/x64/pkg/tool/linux_amd64/vet ache/go/1.25.6/xbash -I ache/go/1.25.6/x--noprofile ache/go/1.25.6/x64/pkg/tool/linux_amd64/vet` (dns block)
Co-autho--format=format:%H %ct %D ache/go/1.25.6/x64/pkg/tool/linux_amd64/vet ache/go/1.25.6/x/opt/hostedtoolcache/go/1.25.6/x64/pkg/tool/linux_amd64/compile -I` (dns block)
Co-autho--format=format:%H %ct %D ache/go/1.25.6/x64/pkg/tool/linux_amd64/vet ache/go/1.25.6/x/opt/hostedtoolcache/go/1.25.6/x64/pkg/tool/linux_amd64/compile -I` (dns block)
Original prompt
This section details on the original issue you should resolve
<issue_title>[go-fan] Go Module Review: BurntSushi/toml</issue_title>
<issue_description># 🐹 Go Fan Report: github.com/BurntSushi/toml
Module Overview
BurntSushi/toml is the de-facto standard TOML parser for Go with 4,893 stars and active maintenance. This library provides a reflection-based API for encoding and decoding TOML configuration files, implementing the TOML specification with excellent compliance. The project is currently using v1.6.0 (released December 18, 2025).
Current Usage in gh-aw-mcpg
The TOML library has a focused, minimal footprint in the gateway:
internal/config/config_core.go)toml.Decode()- Single usage for parsing TOML configuration into Go structsCurrent implementation (lines 97-98 of
config_core.go):Improved version:
2. Detect Configuration Typos with Meta API (10 minutes)
Impact: High - Catch common mistakes like
protinstead ofportEffort: Low - Add 5 lines of code
Catches errors like:
prot = 3000→ warns aboutprot, suggestsportstartup_timout = 60→ warns aboutstartup_timout, suggestsstartup_timeout3. Use Streaming Decoder (10 minutes)
Impact: Medium - Better memory efficiency
Effort: Low - Replace file read with decoder