feat: cap slog group nesting depth at 10 levels#26
Merged
Conversation
Add depth parameter to appendAttr() to prevent unbounded recursion from pathological nested slog group attributes. Groups beyond depth 10 are replaced with a placeholder value.
📝 WalkthroughWalkthroughAdds a recursion depth parameter to attribute flattening in the slog wrapper and enforces a max group nesting limit; deeply nested groups are replaced with a placeholder instead of being expanded. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Adds a safety cap to the slog bridge attribute flattener to avoid unbounded recursion when handling pathologically deep nested slog group attributes.
Changes:
- Add
maxGroupDepthand adepthparameter toappendAttrto cap nested slog group recursion at 10 levels. - Emit a
"[nested group depth exceeded]"placeholder when the depth cap is hit. - Add
TestAppendAttr_DeepNestingto verify deep nesting is capped.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| wrap/slogwrap.go | Implements a max nesting depth check when flattening slog group attributes. |
| wrap/slogwrap_test.go | Adds coverage to ensure deep nested groups are capped and a placeholder is emitted. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
wrap/slogwrap.go (1)
129-150: Extract the depth-exceeded placeholder into a shared constant.The placeholder text is duplicated between implementation and tests. A shared constant avoids drift if wording changes later.
♻️ Proposed refactor
-const maxGroupDepth = 10 +const ( + maxGroupDepth = 10 + nestedGroupDepthExceededMessage = "[nested group depth exceeded]" +) @@ if depth >= maxGroupDepth { - return append(args, key, "[nested group depth exceeded]") + return append(args, key, nestedGroupDepthExceededMessage) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wrap/slogwrap.go` around lines 129 - 150, The placeholder string "[nested group depth exceeded]" is duplicated; extract it into a shared constant (e.g., groupDepthExceededPlaceholder) and use it in appendAttr (which checks depth against maxGroupDepth) instead of the inline literal; update any tests that assert this message to reference the new constant so wording stays synchronized, and ensure the constant is exported or placed in a package-accessible location used by both the implementation and tests.wrap/slogwrap_test.go (1)
411-458: Add explicit boundary tests at the cutoff (10 and 11 levels).This test is great for pathological depth, but adding exact-threshold cases will better guard against off-by-one regressions around
maxGroupDepth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wrap/slogwrap_test.go` around lines 411 - 458, Add two explicit unit tests around the grouping cutoff: one that builds exactly maxGroupDepth levels (should NOT produce the "[nested group depth exceeded]" placeholder and should resolve the leaf) and one that builds maxGroupDepth+1 levels (should produce the placeholder and should NOT resolve the full deep leaf). Reuse the same pattern as TestAppendAttr_DeepNesting (create nested slog.Group chains, create a slog.Record, call ToSlogHandler(h).Handle, inspect cl.lastEntry().Args) but construct one tree of depth equal to maxGroupDepth and another of depth equal to maxGroupDepth+1; assert presence/absence of the placeholder and that the leaf resolution behavior matches expectations. Ensure tests reference the handler conversion flow (ToSlogHandler, Handle) and the same placeholder string "[nested group depth exceeded]" so they guard off-by-one regressions around the maxGroupDepth cutoff.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wrap/slogwrap_test.go`:
- Around line 411-458: Add two explicit unit tests around the grouping cutoff:
one that builds exactly maxGroupDepth levels (should NOT produce the "[nested
group depth exceeded]" placeholder and should resolve the leaf) and one that
builds maxGroupDepth+1 levels (should produce the placeholder and should NOT
resolve the full deep leaf). Reuse the same pattern as
TestAppendAttr_DeepNesting (create nested slog.Group chains, create a
slog.Record, call ToSlogHandler(h).Handle, inspect cl.lastEntry().Args) but
construct one tree of depth equal to maxGroupDepth and another of depth equal to
maxGroupDepth+1; assert presence/absence of the placeholder and that the leaf
resolution behavior matches expectations. Ensure tests reference the handler
conversion flow (ToSlogHandler, Handle) and the same placeholder string "[nested
group depth exceeded]" so they guard off-by-one regressions around the
maxGroupDepth cutoff.
In `@wrap/slogwrap.go`:
- Around line 129-150: The placeholder string "[nested group depth exceeded]" is
duplicated; extract it into a shared constant (e.g.,
groupDepthExceededPlaceholder) and use it in appendAttr (which checks depth
against maxGroupDepth) instead of the inline literal; update any tests that
assert this message to reference the new constant so wording stays synchronized,
and ensure the constant is exported or placed in a package-accessible location
used by both the implementation and tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2aeaf92a-c5d1-49a0-8f39-063e060aeca0
📒 Files selected for processing (2)
wrap/slogwrap.gowrap/slogwrap_test.go
… bump Go 1.25.9 Address review: extract GroupDepthExceededPlaceholder const to avoid duplicated string literal. Derive test nesting depth from maxGroupDepth so the test adapts if the cap changes.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Address review: make groupDepthExceededPlaceholder unexported (not part of public API). Assert leaf value doesn't appear at all when nesting exceeds cap, not just at full depth.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
wrap/slogwrap_test.go (1)
411-454: Strengthen boundary assertion to catch off-by-one depth regressions.Current checks confirm truncation happened, but not where it happened. Adding an expected placeholder key assertion would lock in the exact cap boundary.
Proposed test hardening
func TestAppendAttr_DeepNesting(t *testing.T) { @@ foundPlaceholder := false + foundPlaceholderAtExpectedKey := false foundLeafValue := false @@ + expectedParts := make([]string, maxGroupDepth+1) + for i := 0; i <= maxGroupDepth; i++ { + expectedParts[i] = fmt.Sprintf("g%d", i) + } + expectedPlaceholderKey := strings.Join(expectedParts, ".") + for i := start; i < len(entry.Args)-1; i += 2 { + k, _ := entry.Args[i].(string) v := fmt.Sprint(entry.Args[i+1]) if v == groupDepthExceededPlaceholder { foundPlaceholder = true + if k == expectedPlaceholderKey { + foundPlaceholderAtExpectedKey = true + } } if v == "deep" { foundLeafValue = true } } @@ if !foundPlaceholder { t.Errorf("expected depth-exceeded placeholder in args, got: %v", entry.Args) } + if !foundPlaceholderAtExpectedKey { + t.Errorf("expected placeholder at key %q, got: %v", expectedPlaceholderKey, entry.Args) + } if foundLeafValue { t.Errorf("leaf value should not appear when nesting exceeds maxGroupDepth (%d), got: %v", maxGroupDepth, entry.Args) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@wrap/slogwrap_test.go` around lines 411 - 454, TestAppendAttr_DeepNesting currently only asserts that a depth-exceeded placeholder appears and the leaf value is absent; strengthen it by asserting the placeholder occurs exactly at the expected group boundary. In the TestAppendAttr_DeepNesting test, after building the nested groups and getting entry.Args, locate the pair where the value equals groupDepthExceededPlaceholder and assert the corresponding key equals fmt.Sprintf("g%d", maxGroupDepth) (use maxGroupDepth and groupDepthExceededPlaceholder to compute expected key/value); keep the existing assertion that "deep" does not appear. This ensures the truncation happens at the exact cap and will catch off-by-one regressions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@wrap/slogwrap_test.go`:
- Around line 411-454: TestAppendAttr_DeepNesting currently only asserts that a
depth-exceeded placeholder appears and the leaf value is absent; strengthen it
by asserting the placeholder occurs exactly at the expected group boundary. In
the TestAppendAttr_DeepNesting test, after building the nested groups and
getting entry.Args, locate the pair where the value equals
groupDepthExceededPlaceholder and assert the corresponding key equals
fmt.Sprintf("g%d", maxGroupDepth) (use maxGroupDepth and
groupDepthExceededPlaceholder to compute expected key/value); keep the existing
assertion that "deep" does not appear. This ensures the truncation happens at
the exact cap and will catch off-by-one regressions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d249278-077f-4c14-9c09-2eeeff4e89ab
📒 Files selected for processing (3)
go.modwrap/slogwrap.gowrap/slogwrap_test.go
✅ Files skipped from review due to trivial changes (1)
- go.mod
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
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
appendAttr()to prevent unbounded recursion from pathological nested slog group attributes[nested group depth exceeded]placeholderTest plan
TestAppendAttr_DeepNestingverifies 15-level nesting is capped at 10make testandmake lintcleanSummary by CodeRabbit
Bug Fixes
Tests
Chores