fix: return real defaults when codeowners.toml fails to parse - #184
fix: return real defaults when codeowners.toml fails to parse#184asyncawaitpromise wants to merge 1 commit into
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the 'Approval Retention' feature, which allows users to configure whether existing approvals are retained across specific types of changes, such as whitespace, comments, formatting, string literals, or renames. The implementation includes updates to the configuration parser, new helper methods for flag resolution, comprehensive unit and integration tests, and updated documentation in the README. Since there are no review comments, I have no additional feedback to provide.
ReadConfig handed the same default Config to the parser and to its own error paths. toml.Unmarshal merges into the pointer fields already present, so a file which failed halfway left its partially parsed values in the instance the error path then returned. The caller logged "using default config" and carried on with whatever the parser had managed to read. A malformed file could therefore turn enforcement off, or widen admin bypass, while the logs said defaults were in force. A fresh instance per call fixes it: the parser gets its own, and every error path builds another. The nil-section fixups go away with it, since the defaults are no longer shared and cannot be clobbered.
0572660 to
2c52723
Compare
asyncawaitpromise
changed the title
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking opportunity to strengthen the malformed-config regression test across every default field. The implementation discards the parser-mutated object on failure and returns a fresh default; the remaining concern is limited to incomplete regression coverage for other security-relevant fields. Files Needing Attention: internal/config/config_test.go
|
| Filename | Overview |
|---|---|
| internal/config/config.go | Correctly separates the parser target from fresh error-path defaults while preserving successful partial-config behavior. |
| internal/config/config_test.go | Adds focused regression coverage, but checks only two nested sections rather than the complete pristine-default invariant. |
Reviews (1): Last reviewed commit: "fix: return real defaults when codeowner..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if config.Enforcement == nil { | ||
| t.Fatal("expected Enforcement to be set") | ||
| } | ||
| if config.Enforcement.Approval || !config.Enforcement.FailCheck { | ||
| t.Errorf("Enforcement: expected default {Approval:false FailCheck:true}, got %+v", *config.Enforcement) | ||
| } | ||
| if config.ApprovalRetention == nil { | ||
| t.Fatal("expected ApprovalRetention to be set") | ||
| } | ||
| if config.ApprovalRetention.Enabled { | ||
| t.Error("ApprovalRetention.Enabled: expected default false") | ||
| } |
There was a problem hiding this comment.
The malformed-config test checks only Enforcement and ApprovalRetention.Enabled, leaving fields such as AdminBypass, review limits, and slice-valued defaults outside the stated all-fields invariant. Comparing the complete result with a pristine default would prevent an untested field from leaking through a future regression while this test still passes.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Related PR(s)
Stacked on #183. Independent of the retention feature itself — the bug it fixes predates that work.
Summary / Background
ReadConfighanded the same defaultConfigto the TOML parser and to its own error paths.toml.Unmarshalmerges into the pointer fields already present, so a file which failed halfway left its partially parsed values in the instance the error path then returned.The caller logs
using default configand carries on — with whatever the parser managed to read before it failed.Why it matters
A malformed
codeowners.tomlcould turn enforcement off, or widen admin bypass, while the logs said defaults were in force. The config is a security boundary, so an error path has to return values that never reflect anything read from the file.The fix
A fresh instance per call. The parser gets its own, and every error path builds another. The nil-section fixups go away with it, since the defaults are no longer shared and cannot be clobbered.
Note that a shallow struct copy is not enough here — the pointer fields would still be shared, which is exactly what go-toml merges into.
Verification
A test parses a malformed file and asserts every returned field equals the real default. Reverting the fix turns it red.