add model-cli config command with INI file support#892
add model-cli config command with INI file support#892ericcurtin wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- In
unescapeSubsectionthe comment says unknown backslash escapes are an error, but the implementation silently drops the backslash (matching git); please update the comment to accurately describe the behavior or change the code to match the documented contract. - In
parseValuethe comment mentions line continuation via trailing backslash, but the function currently just treats a trailing backslash as end-of-value with no continuation; consider either implementing proper multi-line continuation or updating the comment to avoid implying continuation is supported.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `unescapeSubsection` the comment says unknown backslash escapes are an error, but the implementation silently drops the backslash (matching git); please update the comment to accurately describe the behavior or change the code to match the documented contract.
- In `parseValue` the comment mentions line continuation via trailing backslash, but the function currently just treats a trailing backslash as end-of-value with no continuation; consider either implementing proper multi-line continuation or updating the comment to avoid implying continuation is supported.
## Individual Comments
### Comment 1
<location path="cmd/cli/gitconfig/gitconfig.go" line_range="75-84" />
<code_context>
+ var entries []Entry
+ var section, subsection string
+
+ scanner := bufio.NewScanner(bytes.NewReader(data))
+ lineNum := 0
+ for scanner.Scan() {
+ lineNum++
+ line := scanner.Text()
+ trimmed := strings.TrimSpace(line)
+
+ // Empty line or comment.
+ if trimmed == "" || trimmed[0] == '#' || trimmed[0] == ';' {
+ continue
+ }
+
+ if trimmed[0] == '[' {
+ // Section header.
+ var err error
+ section, subsection, err = parseSectionHeader(trimmed)
+ if err != nil {
+ return nil, fmt.Errorf("line %d: %w", lineNum, err)
+ }
+ continue
+ }
+
+ // Key-value (or boolean key).
+ if section == "" {
+ return nil, fmt.Errorf("line %d: key outside of section", lineNum)
+ }
+ key, value, err := parseKeyValue(line)
+ if err != nil {
+ return nil, fmt.Errorf("line %d: %w", lineNum, err)
+ }
+ canonical := canonicalKey(section, subsection, key)
+ entries = append(entries, Entry{Key: canonical, Value: value})
+ }
+ return entries, scanner.Err()
+}
+
</code_context>
<issue_to_address>
**suggestion:** Consider guarding against very long lines when using bufio.Scanner
`bufio.Scanner` limits token size to 64KB; lines longer than that will cause `scanner.Err()` to be `ErrTooLong`. If long config values are possible (e.g., certificates, large tokens), consider increasing the buffer with `scanner.Buffer(..., maxCapacity)` and handling `ErrTooLong` explicitly rather than treating it as a generic parse error.
Suggested implementation:
```golang
import (
"bufio"
"bytes"
"errors"
"fmt"
"io"
"os"
"path/filepath"
"strings"
"unicode"
)
const maxConfigLineBytes = 1 << 20 // 1 MiB max line length for gitconfig lines
```
```golang
var entries []Entry
var section, subsection string
scanner := bufio.NewScanner(bytes.NewReader(data))
// Increase the scanner buffer to allow reasonably large config lines
// (e.g. certificates, large tokens). The initial size is small, but
// maxConfigLineBytes is the hard cap.
scanner.Buffer(make([]byte, 0, 64*1024), maxConfigLineBytes)
lineNum := 0
for scanner.Scan() {
lineNum++
line := scanner.Text()
trimmed := strings.TrimSpace(line)
// Empty line or comment.
if trimmed == "" || trimmed[0] == '#' || trimmed[0] == ';' {
continue
}
if trimmed[0] == '[' {
// Section header.
var err error
section, subsection, err = parseSectionHeader(trimmed)
if err != nil {
return nil, fmt.Errorf("line %d: %w", lineNum, err)
}
continue
}
// Key-value (or boolean key).
if section == "" {
return nil, fmt.Errorf("line %d: key outside of section", lineNum)
}
key, value, err := parseKeyValue(line)
if err != nil {
return nil, fmt.Errorf("line %d: %w", lineNum, err)
}
canonical := canonicalKey(section, subsection, key)
entries = append(entries, Entry{Key: canonical, Value: value})
}
if err := scanner.Err(); err != nil {
if errors.Is(err, bufio.ErrTooLong) {
// The failing line is the next line after the last one we
// successfully scanned.
return nil, fmt.Errorf("line %d: config line too long (max %d bytes)", lineNum+1, maxConfigLineBytes)
}
return nil, err
}
return entries, nil
```
</issue_to_address>
### Comment 2
<location path="cmd/cli/gitconfig/gitconfig.go" line_range="206-208" />
<code_context>
+ case inQuotes && c == '"':
+ inQuotes = false
+ i++
+ case c == '\\':
+ if i+1 >= len(raw) {
+ // Trailing backslash = line continuation (we don't handle
+ // multi-line here; treat as end of value).
+ goto done
</code_context>
<issue_to_address>
**issue (bug_risk):** Line-continuation handling with trailing backslash may drop content silently
This logic treats a trailing backslash as "end of value" and discards both the backslash and any intended continuation, which diverges from git’s continuation semantics and can silently truncate values if a user adds a trailing `\`. Consider either implementing continuation by reading the next physical line(s), or treating an unterminated escape at end-of-line as a parse error to avoid silent data loss.
</issue_to_address>
### Comment 3
<location path="cmd/cli/gitconfig/gitconfig.go" line_range="405" />
<code_context>
+ newEntries := transform(append([]Entry(nil), f.entries...))
+
+ data := serialise(newEntries)
+ if err := os.WriteFile(lockPath, data, 0o644); err != nil {
+ return err
+ }
</code_context>
<issue_to_address>
**🚨 issue (security):** Writing with fixed 0644 permissions may weaken existing file permissions
Hardcoding 0644 means a previously more restrictive config (e.g., 0600 with secrets) will be relaxed on rewrite. Consider preserving the existing file mode when the file already exists, or defaulting to a more restrictive mode (e.g., 0600) for per-user configs. You can `os.Stat(f.path)` to reuse its mode and fall back to a secure default otherwise.
</issue_to_address>
### Comment 4
<location path="cmd/cli/commands/config.go" line_range="310-314" />
<code_context>
+ if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
+ return err
+ }
+ if _, err := os.Stat(path); os.IsNotExist(err) {
+ f, err2 := os.Create(path)
+ if err2 != nil {
+ return err2
+ }
+ _ = f.Close()
+ }
+
</code_context>
<issue_to_address>
**🚨 suggestion (security):** Newly created config file in `config edit` likely should use restrictive permissions
`os.Create` will respect the umask but will usually produce a file around 0644, which is too permissive if this config can store tokens or other sensitive data. Please create the file with an explicit 0600 mode (e.g., via `os.OpenFile` with the desired permissions) so it behaves like a private user config file.
```suggestion
// Ensure the file (and its parent directory) exist so the editor
// has something to open.
if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil {
return err
}
if _, err := os.Stat(path); os.IsNotExist(err) {
f, err2 := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, 0o600)
if err2 != nil {
return err2
}
_ = f.Close()
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| case c == '\\': | ||
| if i+1 >= len(raw) { | ||
| // Trailing backslash = line continuation (we don't handle |
There was a problem hiding this comment.
issue (bug_risk): Line-continuation handling with trailing backslash may drop content silently
This logic treats a trailing backslash as "end of value" and discards both the backslash and any intended continuation, which diverges from git’s continuation semantics and can silently truncate values if a user adds a trailing \. Consider either implementing continuation by reading the next physical line(s), or treating an unterminated escape at end-of-line as a parse error to avoid silent data loss.
There was a problem hiding this comment.
Code Review
This pull request introduces a new config command to the CLI, enabling users to read and write configuration values in a git-style INI format. The implementation includes a new gitconfig package for parsing and serializing these files, along with comprehensive tests. Key feedback includes a critical security concern regarding the preservation of file permissions during atomic writes, a parsing limitation where trailing comments on section headers cause failures, and an issue with the edit command failing when environment variables like EDITOR contain arguments.
| if err := os.WriteFile(lockPath, data, 0o644); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
The writeAtomic function uses a hardcoded file mode of 0o644 when writing the config file. This is a security risk as it can make sensitive configuration files (e.g., those containing API keys or tokens) world-readable, even if the original file had more restrictive permissions (like 0600). The implementation should preserve the permissions of the existing file if it exists.
| if err := os.WriteFile(lockPath, data, 0o644); err != nil { | |
| return err | |
| } | |
| mode := os.FileMode(0o644) | |
| if info, err := os.Stat(f.path); err == nil { | |
| mode = info.Mode() | |
| } | |
| if err := os.WriteFile(lockPath, data, mode); err != nil { | |
| return err | |
| } |
| } | ||
|
|
||
| //nolint:gosec // editor and path are user-controlled inputs, which is intentional | ||
| editorCmd := exec.CommandContext(cmd.Context(), editor, path) |
There was a problem hiding this comment.
The edit command uses exec.CommandContext with the editor string directly as the executable name. This will fail if the VISUAL or EDITOR environment variables contain arguments (e.g., EDITOR="code --wait" or EDITOR="vim -R"). To support common user configurations, the command string should be parsed into an executable and its arguments before being passed to exec.CommandContext.
9a8c63e to
741374a
Compare
741374a to
a6883d8
Compare
Introduce 'model-cli config' as a new top-level command with an interface and file format inspired by, but not referencing, 'git config'. - New cmd/cli/iniconfig package: parses and writes INI-style config files (section headers, subsections, boolean keys, inline comments, backslash escapes, quoted values, UTF-8 BOM). Writes are atomic via .lock + rename. - New 'config' command with subcommands: get, set, unset, list, edit. All subcommands accept --global (default per XDG_CONFIG_HOME or ~/.config/model-runner/config), --system (/etc/model-runner/config), and --file/-f flags. - Remove the 'config' alias from 'configure' to avoid a name collision; 'configure' remains hidden and undocumented for existing callers. - 'config' requires no running model-runner instance (pure local file I/O) and is registered outside the withStandaloneRunner group. - Parser: handle trailing comments on section headers ([core] # comment), raise a clear error on lines exceeding 1 MiB, preserve existing file permissions on write (default 0600 for new files). - Editor: split VISUAL/EDITOR on whitespace to support values like 'code --wait'. - Regenerate CLI reference docs.
a6883d8 to
4017e46
Compare
| } | ||
| if count > 1 { | ||
| return "", fmt.Errorf("only one of --global, --system, or --file may be specified") | ||
| } |
There was a problem hiding this comment.
can do something like
count := 0
for _, configFlag := range []bool{global, system, file != ""} {
if configFlag{
count++
}
}
if count > 1 {
return "", fmt.Errorf("only one of --global, --system, or --file may be specified")
}
| return nil | ||
| } | ||
|
|
||
| v, ok := f.Get(key) |
There was a problem hiding this comment.
can remove duplicate code and logic by using a slice to hold the values
key := args[0]
var vals []string
if showAll {
vals = f.GetAll(key)
} else if v, ok := f.Get(key); ok {
vals = []string{v}
}
if len(vals) == 0 {
if hasDefault {
cmd.Println(defaultVal)
return nil
}
return fmt.Errorf("key not found: %s", key)
}
for _, v := range vals {
if showOrigin {
cmd.Printf("file:%s\t%s\n", path, v)
} else {
cmd.Println(v)
}
}
return nil
},
Introduce 'model-cli config' as a new top-level command with an interface
and file format inspired by, but not referencing, 'git config'.
(section headers, subsections, boolean keys, inline comments, backslash
escapes, quoted values, UTF-8 BOM). Writes are atomic via .lock + rename.
All subcommands accept --global (default per XDG_CONFIG_HOME or
~/.config/model-runner/config), --system (/etc/model-runner/config),
and --file/-f flags.
'configure' remains hidden and undocumented for existing callers.
and is registered outside the withStandaloneRunner group.
raise a clear error on lines exceeding 1 MiB, preserve existing file
permissions on write (default 0600 for new files).
'code --wait'.