Add a settings loader - #2
Conversation
Reads key=value files. Unknown keys are ignored so an older binary can read a newer file without failing.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| n, _ := strconv.Atoi(strings.TrimSpace(value)) | ||
| s.Retries = n | ||
| case "verbose": | ||
| s.Verbose = strings.TrimSpace(value) == "true" |
There was a problem hiding this comment.
🟠 High config/load.go:38
Load silently accepts invalid values: verbose=ture or verbose=yes returns Settings{Verbose:false} with a nil error, while retries=three returns Settings{Retries:0} with a nil error, overriding the default of 3. The Atoi error is discarded and non-true verbose values are treated as false; return parsing errors instead.
| n, _ := strconv.Atoi(strings.TrimSpace(value)) | |
| s.Retries = n | |
| case "verbose": | |
| s.Verbose = strings.TrimSpace(value) == "true" | |
| n, err := strconv.Atoi(strings.TrimSpace(value)) | |
| if err != nil { | |
| return s, err | |
| } | |
| s.Retries = n | |
| case "verbose": | |
| v := strings.TrimSpace(value) | |
| if v != "true" && v != "false" { | |
| return s, strconv.ErrSyntax | |
| } | |
| s.Verbose = v == "true" |
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/config/load.go around lines 38-41:
`Load` silently accepts invalid values: `verbose=ture` or `verbose=yes` returns `Settings{Verbose:false}` with a nil error, while `retries=three` returns `Settings{Retries:0}` with a nil error, overriding the default of `3`. The `Atoi` error is discarded and non-`true` verbose values are treated as false; return parsing errors instead.
| continue | ||
| } | ||
|
|
||
| key, value, _ := strings.Cut(line, "=") |
There was a problem hiding this comment.
🟡 Medium config/load.go:33
A malformed known-key line such as name is accepted as valid input and clears Settings.Name, so Load reports success without letting callers distinguish it from name=. The found result from strings.Cut is ignored; validate it and return a parse error when = is missing.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @internal/config/load.go around line 33:
A malformed known-key line such as `name` is accepted as valid input and clears `Settings.Name`, so `Load` reports success without letting callers distinguish it from `name=`. The `found` result from `strings.Cut` is ignored; validate it and return a parse error when `=` is missing.
|
@coderabbitai review |
|
@macroscopeapp review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1f03088834
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| n, _ := strconv.Atoi(strings.TrimSpace(value)) | ||
| s.Retries = n |
There was a problem hiding this comment.
When retries is missing a value, nonnumeric, or overflows an int, strconv.Atoi returns an error that is discarded, and Retries is then set to zero. This silently replaces the documented default of 3 (or an earlier valid value), while Load returns a nil error, potentially disabling retries because of a malformed setting; propagate the parsing error or preserve the previous/default value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Merged into Invalid retries value silently becomes 0, overriding the documented default of 3 (internal/config/load.go:38), also reported by macroscopeapp, chatgpt-codex-connector, copilot-pull-request-reviewer.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #2 (comment)
There was a problem hiding this comment.
Merged into Invalid retries value silently becomes 0, overriding the documented default of 3 (internal/config/load.go:38), also reported by macroscopeapp, chatgpt-codex-connector, copilot-pull-request-reviewer.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #2 (comment)
There was a problem hiding this comment.
Merged into Invalid retries value silently resets to 0, overriding the documented default of 3 (internal/config/load.go:38), also reported by macroscopeapp, chatgpt-codex-connector, copilot-pull-request-reviewer.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #2 (comment)
There was a problem hiding this comment.
Pull request overview
Adds an internal/config package for loading simple key=value settings files into a Settings struct, with defaults and forward-compatible handling of unknown keys.
Changes:
- Introduces
config.Settingsandconfig.Load(path)to parse settings files, skip blank lines /#comments, and ignore unknown keys. - Applies a default for
Retries(3) and parsesretries/verbose/namevalues from the file. - Adds initial unit tests covering key reading and comment ignoring.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/config/load.go | Implements Settings and Load to parse key=value settings with defaults and ignored unknown keys. |
| internal/config/load_test.go | Adds tests for parsing known keys and skipping # comment lines. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| case "retries": | ||
| n, _ := strconv.Atoi(strings.TrimSpace(value)) | ||
| s.Retries = n |
There was a problem hiding this comment.
Merged into Invalid retries value silently becomes 0, overriding the documented default of 3 (internal/config/load.go:38), also reported by macroscopeapp, chatgpt-codex-connector, copilot-pull-request-reviewer.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #2 (comment)
There was a problem hiding this comment.
Merged into Invalid retries value silently becomes 0, overriding the documented default of 3 (internal/config/load.go:38), also reported by macroscopeapp, chatgpt-codex-connector, copilot-pull-request-reviewer.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #2 (comment)
There was a problem hiding this comment.
Merged into Invalid retries value silently resets to 0, overriding the documented default of 3 (internal/config/load.go:38), also reported by macroscopeapp, chatgpt-codex-connector, copilot-pull-request-reviewer.
Resolving this thread as a duplicate — the finding itself is not dismissed, and reopening this thread is the right move if the merge is wrong.
See the consolidated review: #2 (comment)
| "strings" | ||
| ) | ||
|
|
||
| // Settings holds what a settings file described. |
| func TestLoadIgnoresComments(t *testing.T) { | ||
| got, _ := Load(write(t, "# a comment\nname = app\n")) | ||
| if got.Name != "app" { | ||
| t.Errorf("got %+v", got) | ||
| } | ||
| } |
Review reconciled — 5 findings from 3 reviewersFindingsMAJOR — Invalid MINOR — Non-"true" MINOR — A known-key line without MINOR — No test exercises the NIT — CoverageAbsent: coderabbitai, baz-reviewer — this verdict is incomplete, not clean. Verdict: incomplete |
githail
Bot
added
githail:incomplete
githail:reconciling
and removed
githail:reconciling
githail:incomplete
labels
User description
Reads simple
key=valuesettings files.Unknown keys are ignored deliberately, so an older binary can read a file written by a newer one without failing. Defaults
retriesto 3.Generated description
Below is a concise technical summary of the changes proposed in this PR:
Add a
config.Loadsettings loader that parses simplekey=valuefiles intoSettings, applying defaults and ignoring unknown keys for forward compatibility. Cover key parsing and comment handling with focused tests.Latest Contributors(1)
Summary by cubic
Adds a
configpackage that loads simplekey=valuesettings files. Unknown keys are ignored so older binaries can read newer files, andretriesdefaults to 3.Written for commit 1f03088. Summary will update on new commits.
Note
Add
config.Loadto parse key=value settings filesAdds a
Settingsstruct (Name,Retries,Verbose) and aLoad(path)function that reads simplekey=valuefiles, skipping blank lines and#comments. Recognized keys arename,retries(parsed viastrconv.Atoi), andverbose(settrueonly when value equals"true"); unknown keys are ignored.Retriesdefaults to3. Includes tests for reading keys and ignoring comments.retriesparse failure,Retriesis set to theAtoizero-value (0) rather than the default3; on file read error,Loadreturns the default-initializedSettings(Retries=3) alongside the error.📊 Macroscope summarized 1f03088. 1 file reviewed, 3 issues evaluated, 0 issues filtered, 2 comments posted
🗂️ Filtered Issues