fix: rules.Merge no longer panics when the base declares no domains (iss-187) - #205
Merged
Merged
Conversation
rules.Merge wrote override domain keys straight into out.Domains
without ever allocating that map, because cloneRuleSet deliberately
preserves a nil Domains map when the source has none. A base such as
RuleSet{SchemaVersion: 1} — which Validate accepts — panicked with
"assignment to entry in nil map" the moment the overlay carried a
domain, contradicting Merge's own doc comment ("New domain keys are
added"). Not reachable today (RuleSet.Load always merges onto
Defaults(), which always has a populated map), but a latent defect in
an exported API contract.
Allocate out.Domains before the override loop when it's nil and the
overlay has at least one key, matching the idiom guard.Merge already
uses for out.Entries. cloneRuleSet keeps its nil-preserving semantics
for every other caller.
Resolves iss-187.
Assisted-by: Claude:claude-opus-4-8
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
2 participants
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.
Fixes iss-187 (minor), surfaced during a bug-hunt loop sweep (round 1) and verified with a failing test.
The bug
rules.Merge(base, over RuleSet) RuleSet(internal/core/rules/rules.go) wrote override domain keys straight intoout.Domainswithout ever allocating that map.cloneRuleSetdeliberately preserves a nilDomainsmap when the source has none, so a base such asRuleSet{SchemaVersion: 1}— aValidate-accepted, valid RuleSet — panicked withassignment to entry in nil mapthe moment the overlay carried a domain key, contradictingMerge's own doc comment ("New domain keys are added"), which promises no such precondition.Not reachable in production today: the sole call site,
Load, always callsMerge(Defaults(), over), andDefaults()always has a populatedDomainsmap. This is a latent defect in an exported API contract — it goes live the moment any caller merges onto a non-Defaults()base (e.g. a future multi-tier overlay starting from an emptyRuleSet).The fix
Mergenow allocatesout.Domains(viamake) before the override loop when it's nil and the overlay has at least one key — matching the idiom the sibling loaderguard.Merge(internal/core/guard/config.go) already uses forout.Entries.cloneRuleSetis untouched, so its nil-preserving semantics hold for every other current and future caller.Evidence
internal/core/rules/rules.go— the allocation guard inMergeinternal/core/rules/rules_test.go—TestMergeNilBaseDomainsAddsNewKeys, watched failing on pre-fix code for the claimed reason (panic: assignment to entry in nil map) and passing after.abcd/work/issues/resolved/iss-187-rules-merge-nil-domains-panic.md— resolved viaabcd capture resolveCHANGELOG.md,.abcd/work/DECISIONS.md— updatedAn independent adversarial correctness review of the full diff (fresh subagent, no context from the fix) walked all four base/overlay nil-combinations, reverted the fix locally to confirm the test fails for the exact claimed reason, checked the sole call site and
cloneRuleSetfor regressions, and checked for concurrency issues. Verdict: MERGE. No blocking findings.make preflightgreen;gofmt -l .clean;go run ./cmd/record-lint0 blockers.