refactor(config): add LoadTOML, remove SetXxxForTesting methods#657
Merged
Conversation
Replace 9 SetXxxForTesting methods with a single LoadTOML(data string) method that parses a TOML snippet and rebuilds derived fields. Refactor Load() to share the unmarshal + post-processing core via applyTOML(), preserving atomic swap semantics on reload failure.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughRefactored config loading: introduced LoadTOML/applyTOML, moved TOML parsing and derived-field rebuilds into helpers, and made Load() snapshot and restore c.vals on failure while checking schema against applied values. Removed multiple exported testing-only Set*ForTesting helpers; tests now load TOML strings. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
pkg/config/config_test.go (1)
1209-1243: Add one negative-path test forLoadTOMLstate preservation.These updates validate successful TOML application well, but they don’t yet assert behavior after
LoadTOMLfailure (e.g., invalid TOML should not leave partially-updated allow/block state). A regression test here would protect the new apply pipeline.As per coding guidelines, `**/*.go`: Write tests for all new code — see TESTING.md and pkg/testing/README.md.🧪 Suggested test skeleton
+func TestLoadTOML_InvalidInput_DoesNotMutateAllowLists(t *testing.T) { + t.Parallel() + + cfg := &Instance{} + require.NoError(t, cfg.LoadTOML(`[zapscript] +allow_execute = ["^echo$"]`)) + assert.True(t, cfg.IsExecuteAllowed("echo")) + + err := cfg.LoadTOML(`[zapscript +allow_execute = ["^rm$"]`) // invalid TOML + require.Error(t, err) + + // Prior state should remain intact after failure. + assert.True(t, cfg.IsExecuteAllowed("echo")) + assert.False(t, cfg.IsExecuteAllowed("rm")) +}Also applies to: 1333-1335, 1381-1383, 1428-1430
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/config/config_test.go` around lines 1209 - 1243, Add a negative-path unit test to ensure Instance.LoadTOML does not mutate existing state on failure: create an Instance (cfg), initialize it with a known allow/block state (e.g., LoadTOML with allow_execute = ["echo"] and assert IsExecuteAllowed("echo") is true), then call cfg.LoadTOML with deliberately invalid TOML (expect require.Error) and finally re-assert the original IsExecuteAllowed/IsExecuteBlocked expectations remain unchanged; reference the Instance struct, its LoadTOML method, and IsExecuteAllowed helper when adding this test.pkg/zapscript/input_test.go (1)
74-78: Consider extracting a shared TOML config helper in this test file.The repeated
cfg := &config.Instance{} + require.NoError(t, cfg.LoadTOML(...))blocks are consistent but duplicated heavily; a tiny helper would reduce maintenance noise.♻️ Example refactor
+func mustLoadInputConfig(t *testing.T, toml string) *config.Instance { + t.Helper() + cfg := &config.Instance{} + require.NoError(t, cfg.LoadTOML(toml)) + return cfg +} ... - cfg := &config.Instance{} - require.NoError(t, cfg.LoadTOML(` + cfg := mustLoadInputConfig(t, ` [zapscript.input] mode = "combos" block = [] -`)) +`)Also applies to: 100-103, 116-119, 143-147, 163-167, 178-182, 205-209, 251-255, 274-277, 300-303, 325-328, 350-353
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/zapscript/input_test.go` around lines 74 - 78, Several tests duplicate cfg := &config.Instance{} and require.NoError(t, cfg.LoadTOML(...)); extract a small helper like newCfgFromTOML(t testing.TB, toml string) *config.Instance that constructs &config.Instance{}, calls require.NoError(t, cfg.LoadTOML(toml)), and returns cfg, then replace each duplicated block in pkg/zapscript/input_test.go (occurrences around the require.NoError lines and tests that reference cfg) with calls to newCfgFromTOML to reduce repetition and centralize error handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/config.go`:
- Around line 262-275: Add unit tests that directly exercise Instance.LoadTOML
and compare behavior with Instance.Load: create a baseline Instance with known
fields and derived state (compiled regexes, lookup maps, and methods like
IsUserAllowed/IsIPAllowed etc.), then (1) pass a TOML that only updates a subset
of fields and assert unchanged fields remain the same to verify partial-merge
semantics of LoadTOML; (2) pass invalid TOML to LoadTOML and assert it returns
an error and leaves the Instance state mutated (no rollback), and separately
assert that Load (not LoadTOML) performs rollback on error; and (3) update TOML
for fields that affect compiled regexes and maps and assert those derived
structures are rebuilt and Is*Allowed methods reflect the new rules; reference
Instance.LoadTOML, Instance.applyTOML, Instance.Load, and Is*Allowed helpers to
locate code under test.
- Around line 265-269: LoadTOML currently locks and calls applyTOML which
mutates c.vals in place; because go-toml/v2 can leave structs partially modified
on decode error, change LoadTOML to be defensive by decoding/applying into a
temporary copy and only replacing c.vals on success (or by saving the original
c.vals, calling applyTOML on the copy, and restoring the original on any error).
Ensure you still hold c.mu while swapping the successful new value into c.vals
and reference the existing symbols c.mu, LoadTOML, applyTOML, and c.vals when
making the change.
---
Nitpick comments:
In `@pkg/config/config_test.go`:
- Around line 1209-1243: Add a negative-path unit test to ensure
Instance.LoadTOML does not mutate existing state on failure: create an Instance
(cfg), initialize it with a known allow/block state (e.g., LoadTOML with
allow_execute = ["echo"] and assert IsExecuteAllowed("echo") is true), then call
cfg.LoadTOML with deliberately invalid TOML (expect require.Error) and finally
re-assert the original IsExecuteAllowed/IsExecuteBlocked expectations remain
unchanged; reference the Instance struct, its LoadTOML method, and
IsExecuteAllowed helper when adding this test.
In `@pkg/zapscript/input_test.go`:
- Around line 74-78: Several tests duplicate cfg := &config.Instance{} and
require.NoError(t, cfg.LoadTOML(...)); extract a small helper like
newCfgFromTOML(t testing.TB, toml string) *config.Instance that constructs
&config.Instance{}, calls require.NoError(t, cfg.LoadTOML(toml)), and returns
cfg, then replace each duplicated block in pkg/zapscript/input_test.go
(occurrences around the require.NoError lines and tests that reference cfg) with
calls to newCfgFromTOML to reduce repetition and centralize error handling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6574d71c-5b00-4eea-bd43-276dacd765f7
📒 Files selected for processing (15)
pkg/config/config.gopkg/config/config_test.gopkg/config/configlaunchers.gopkg/config/configlaunchers_test.gopkg/config/configservice.gopkg/config/configsystems.gopkg/platforms/launch_test.gopkg/platforms/shared/steam/client_darwin_test.gopkg/platforms/shared/steam/client_linux_test.gopkg/platforms/shared/steam/client_windows_test.gopkg/service/scan_behavior_test.gopkg/zapscript/http_test.gopkg/zapscript/input_test.gopkg/zapscript/launch_test.gopkg/zapscript/utils_test.go
💤 Files with no reviewable changes (3)
- pkg/config/configservice.go
- pkg/config/configlaunchers.go
- pkg/config/configsystems.go
…lper - Add tests for partial-merge, invalid TOML rollback, regex rebuilding, and Load() schema error rollback - Make LoadTOML defensive: save/restore c.vals on decode error - Extract newCfgFromTOML helper in input_test.go to reduce repetition
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/config/config_test.go`:
- Around line 1284-1308: Update TestLoad_RollbackOnSchemaError to make the
rollback observable by changing a decoded field in the bad config file: when you
write the bad config (currently only setting config_schema = 9999), also include
a modified allow_execute entry (e.g. allow_execute = ["rm"]) so Load() will
decode user values before schema validation fails; after cfg.Load() returns an
error assert the running config is unchanged by checking
cfg.IsExecuteAllowed("echo") is still true and the new value (e.g. "rm") is not
allowed. Target the TestLoad_RollbackOnSchemaError test and the code paths
around cfg.Load() and cfg.IsExecuteAllowed to add the failing config write and
the extra assertion.
- Around line 1232-1248: Replace the current failing TOML snippet in
TestLoadTOML_InvalidTOMLPreservesState with a "late-failure" TOML that first
decodes a valid field and then triggers a type error (so LoadTOML applies an
earlier field then fails later); call cfg.LoadTOML with that new snippet,
require an error, assert cfg.IsExecuteAllowed("echo") is still true, and assert
the invalid candidate was not applied (e.g., a new command is not allowed). Keep
references to the same test function TestLoadTOML_InvalidTOMLPreservesState, the
cfg.LoadTOML call, and cfg.IsExecuteAllowed checks to validate rollback
behavior.
- Around line 1250-1282: The test only verifies allow_execute was rebuilt after
the second cfg.LoadTOML call; extend the assertions to re-check the other
derived caches that are rebuilt on the same path: after calling
cfg.LoadTOML(`...allow_execute = ["rm"]`), also assert
cfg.IsHTTPAllowed("https://example.com/foo"), cfg.IsCommandBlocked("http.get"),
and cfg.IsRunAllowed("**launch:test") (and their negations where appropriate) to
ensure allowHTTPRe, blockCommandSet and allowRunRe were recompiled/retained
correctly alongside IsExecuteAllowed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b03a810c-91c9-43b3-9b6e-7cd428fde2cc
📒 Files selected for processing (3)
pkg/config/config.gopkg/config/config_test.gopkg/zapscript/input_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/config/config.go
- Use type-error TOML for InvalidTOMLPreservesState to test partial decode rollback (not just parse failure) - Add allow_execute to bad schema config in RollbackOnSchemaError so rollback is observable via decoded field - Assert HTTP, block, and run derived fields are retained after partial LoadTOML update in RebuildsDerivedFields
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
LoadTOML(data string) errormethod toconfig.Instancethat parses a TOML snippet and rebuilds all derived fields (compiled regexes, lookup maps)Load()to share unmarshal + post-processing via privateapplyTOML(), preserving atomic swap on reload failureSetXxxForTestingmethods and rewrite ~50 call sites across 15 test files to use declarative TOML snippetsSummary by CodeRabbit
Bug Fixes
New Features
Tests