Skip to content

Commit 2e76934

Browse files
49EHyeon42Yuan325
49EHyeon42
and
Yuan325
authored
fix(config): compare env var offsets in rune space when skipping comments (#3856)
## Description `parseEnv` skipped some `${VAR}` placeholders that were not inside comments, leaving the literal `${VAR}` text as the configuration value with no error or warning. It happened when the config contained comments with multi-byte characters, so a config could start failing after only a comment was edited. The offsets came from two different coordinate spaces. `re.FindAllStringSubmatchIndex` returns byte offsets, while `token.Position.Offset` from the go-yaml lexer is a 1-based rune offset: the scanner converts its input with `[]rune(text)` and starts counting at `1`. For input containing multi-byte characters the two drift apart, and once the drift is large enough a placeholder that sits outside any comment gets an offset that falls inside a later comment token's range. The range end had the same problem, since it was computed with `len(t.Origin)` in bytes. `Origin` also carries the indentation that precedes the `#` while `Position.Offset` already points at the `#`, so the range could extend past the end of the comment. This is a regression from #3807. ### Solution - Track a 1-based rune offset alongside the byte offset at the call site, so both sides of the comparison use the lexer's coordinate space. Matches are ordered, so the input is still only walked once. - Measure the comment length in runes, from the `#`, so the range covers exactly the comment. - Add two `TestParseEnv` cases: one asserting placeholders after a multi-byte comment are substituted, and one asserting placeholders inside a multi-byte comment are still skipped. Verified that the new cases fail without the change and pass with it, and that the existing `cmd/internal` tests, `gofmt` and `go vet` are clean. ## PR Checklist - [x] Make sure you reviewed [CONTRIBUTING.md](https://github.com/googleapis/mcp-toolbox/blob/main/CONTRIBUTING.md) - [x] Make sure to open an issue as a [bug/issue](https://github.com/googleapis/mcp-toolbox/issues/new/choose) before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea - [x] Ensure you have manually reviewed the entire diff before requesting a review - [x] Ensure the tests and linter pass - [x] Code coverage does not decrease (if any source code was changed) - [x] Appropriate docs were updated (if necessary) - [ ] Make sure to add `!` if this involve a breaking change 🛠️ Fixes #3855 Co-authored-by: Yuan Teoh <45984206+Yuan325@users.noreply.github.com>
1 parent 0fe2e30 commit 2e76934

2 files changed

Lines changed: 41 additions & 4 deletions

File tree

‎cmd/internal/config.go‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@ import (
2525
"regexp"
2626
"slices"
2727
"strings"
28+
"unicode/utf8"
2829

2930
"github.com/goccy/go-yaml"
3031
"github.com/goccy/go-yaml/lexer"
@@ -74,11 +75,19 @@ func (p *ConfigParser) parseEnv(input string) (string, error) {
7475
matches := re.FindAllStringSubmatchIndex(input, -1)
7576
var output strings.Builder
7677
lastIndex := 0
78+
// The lexer reports token positions as 1-based rune offsets, while the regexp
79+
// reports byte offsets. Track the rune offset alongside so both use the same
80+
// coordinate space; matches are ordered, so this only walks the input once.
81+
runeOffset := 1
82+
scannedBytes := 0
7783
for _, match := range matches {
7884
start, end := match[0], match[1]
7985

86+
runeOffset += utf8.RuneCountInString(input[scannedBytes:start])
87+
scannedBytes = start
88+
8089
// Skip substitution if the variable is inside a comment
81-
if isInsideComment(tokens, start) {
90+
if isInsideComment(tokens, runeOffset) {
8291
output.WriteString(input[lastIndex:end])
8392
lastIndex = end
8493
continue
@@ -141,11 +150,16 @@ func (p *ConfigParser) parseEnv(input string) (string, error) {
141150
return output.String(), err
142151
}
143152

144-
// isInsideComment checks if the given byte offset in the YAML input is within a comment token.
145-
func isInsideComment(tokens token.Tokens, offset int) bool {
153+
// isInsideComment checks if the given 1-based rune offset in the YAML input is
154+
// within a comment token. Token positions from the lexer are 1-based rune
155+
// offsets, so callers must convert byte offsets before calling this.
156+
func isInsideComment(tokens token.Tokens, runeOffset int) bool {
146157
for _, t := range tokens {
147158
if t.Type == token.CommentType && t.Position != nil {
148-
if offset >= t.Position.Offset && offset < t.Position.Offset+len(t.Origin) {
159+
// Position.Offset points at the "#", but Origin also carries any
160+
// indentation that precedes it, so measure the length from the "#".
161+
length := utf8.RuneCountInString(strings.TrimLeft(t.Origin, " \t"))
162+
if runeOffset >= t.Position.Offset && runeOffset < t.Position.Offset+length {
149163
return true
150164
}
151165
}

‎cmd/internal/config_test.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -139,6 +139,29 @@ func TestParseEnv(t *testing.T) {
139139
},
140140
want: "foo: bar_val\n# ${FOO}\nbaz: qux_val",
141141
},
142+
{
143+
desc: "parse env vars after a comment containing multi-byte characters",
144+
in: "# " + strings.Repeat("é", 30) + "\n" +
145+
"foo: ${BAR}\n" +
146+
"# " + strings.Repeat("é", 30) + "\n" +
147+
"baz: ${QUX}\n",
148+
env: map[string]string{
149+
"BAR": "bar_val",
150+
"QUX": "qux_val",
151+
},
152+
want: "# " + strings.Repeat("é", 30) + "\n" +
153+
"foo: bar_val\n" +
154+
"# " + strings.Repeat("é", 30) + "\n" +
155+
"baz: qux_val\n",
156+
},
157+
{
158+
desc: "skip commented out env var when comment contains multi-byte characters",
159+
in: "# " + strings.Repeat("é", 30) + " ${FOO}\nbar: ${BAR}\n",
160+
env: map[string]string{
161+
"BAR": "bar_val",
162+
},
163+
want: "# " + strings.Repeat("é", 30) + " ${FOO}\nbar: bar_val\n",
164+
},
142165
{
143166
desc: "multiline yaml with mixed comments and env vars",
144167
in: "database: my-db # Another comment in line ${SHOULD_BE_IGNORED}\n" +

0 commit comments

Comments
 (0)