Improvements #63
Improvements #63
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Here's the code health analysis summary for commits Analysis Summary
|
|
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (2)
server/root-handler/handler.go (1)
72-78: Potential nil-pointer dereference on InitializeParams.CapabilitiesThe chain dereferences can panic if any nested capability pointer is nil. Guard each level before reading PrepareSupport.
Apply this diff to harden the check:
- if (*params.Capabilities.TextDocument.Rename.PrepareSupport) == true { + if params != nil && + params.Capabilities.TextDocument != nil && + params.Capabilities.TextDocument.Rename != nil && + params.Capabilities.TextDocument.Rename.PrepareSupport != nil && + *params.Capabilities.TextDocument.Rename.PrepareSupport { // Client supports rename preparation prepareRename := true capabilities.RenameProvider = protocol.RenameOptions{ PrepareProvider: &prepareRename, } }server/handlers/wireguard/indexes/indexes_handlers.go (1)
24-27: Nil Header guard to avoid panics on root sectionssection.Header can be nil for the synthetic root section when AllowRootProperties is enabled. Accessing section.Header.Name will panic. Guard or assign a sentinel name for root.
- sectionName := section.Header.Name + if section.Header == nil { + // Skip synthetic root or assign a sentinel name if needed + continue + } + sectionName := section.Header.Name
🧹 Nitpick comments (27)
server/parsers/ini/ast.go (1)
18-18: Refine PropertyValue.Raw documentation and naming consistency
- In server/parsers/ini/ast.go, add a GoDoc comment to explain semantics of the
Rawfield:// Raw is the original, unnormalized substring from the source (including quotes and escape sequences). Raw string- Consider renaming
Raw→RawValue(or renaming other types’RawValue→Raw) so that INI AST types matchProperty.RawValueandHeader.RawValueelsewhere.- No current JSON/YAML marshaling of
PropertyValuewas found in the codebase. If these AST types will ever be serialized, explicitly omitRawvia a struct tag, for example:Raw string `json:"-"`--- a/server/parsers/ini/ast.go +++ b/server/parsers/ini/ast.go @@ type PropertyValue struct { - Raw string + // Raw is the original, unnormalized substring from the source (including quotes and escape sequences). + Raw string[optional_refactors_recommended]
server/utils/quotes.go (1)
100-111: NormalizeEscapedQuotes works but is O(n²) due to repeated string rebuilds; rewrite to a single-pass builder.Current approach reallocates on each removal. A linear pass avoids churn and keeps semantics identical (handles ", ', and cases like \").
Apply this diff:
-func NormalizeEscapedQuotes(value string) string { - // Search for \" and remove the backslash - for i := 0; i < len(value)-1; i++ { - if value[i] == '\\' && ((value[i+1] == '"') || (value[i+1] == '\'')) { - value = value[:i] + value[i+1:] - i-- // Adjust index after removal - } - } - - return value -} +func NormalizeEscapedQuotes(value string) string { + // Remove a preceding backslash when it escapes a quote (\" or \') + // Single-pass, linear-time implementation to avoid repeated allocations. + if len(value) < 2 { + return value + } + out := make([]byte, 0, len(value)) + for i := 0; i < len(value); i++ { + if value[i] == '\\' && i+1 < len(value) && (value[i+1] == '"' || value[i+1] == '\'') { + // Skip the backslash and keep the quote + out = append(out, value[i+1]) + i++ + continue + } + out = append(out, value[i]) + } + return string(out) +}Optional: broaden tests for single quotes and doubled escapes to pin the intended semantics (see test suggestions in quotes_test.go comment).
server/utils/quotes_test.go (1)
97-108: Good coverage for a basic case; add tests for single quotes and doubled escapes to lock semantics.To prevent regressions and clarify intended behavior, consider adding:
- Single-quote unescape:
it\'s->it's- Double-escaped backslash + quote:
hello \\"world"-> eitherhello "world"(current impl) orhello \"world"(if you prefer preserving one backslash) — choose and lock it in.You could extend with:
func TestNormalizeSingleQuotes(t *testing.T) { input := `it\'s fine` expected := `it's fine` if got := NormalizeEscapedQuotes(input); got != expected { t.Fatalf("expected %q, got %q", expected, got) } } func TestNormalizeDoubleEscapedBackslashQuote(t *testing.T) { // Pick desired behavior and set expected accordingly. input := `hello \\"world"` // If we want full normalization to a bare quote: expected := `hello "world"` // If instead we want to preserve one backslash, set: // expected := `hello \"world"` if got := NormalizeEscapedQuotes(input); got != expected { t.Fatalf("expected %q, got %q", expected, got) } }server/handlers/wireguard/indexes/indexes_test.go (1)
28-30: Strengthen assertion: verify map entry exists and simplify conditionAccessing a map entry without checking existence will silently read a zero-value struct if the key is missing, potentially masking indexing bugs. Also, the boolean comparison can be simplified.
Apply this diff to assert presence and improve readability:
- if !(indexes.AsymmetricRules[config.Sections[0]].PreMissing == true && indexes.AsymmetricRules[config.Sections[0]].PostMissing == false) { - t.Errorf("Expected asymmetric rules for section '%s' to be PreMissing: true, PostMissing: false, got PreMissing: %v, PostMissing: %v", config.Sections[0].Header.Name, indexes.AsymmetricRules[config.Sections[0]].PreMissing, indexes.AsymmetricRules[config.Sections[0]].PostMissing) - } + info, ok := indexes.AsymmetricRules[config.Sections[0]] + if !ok { + t.Fatalf("AsymmetricRules missing entry for section '%s'", config.Sections[0].Header.Name) + } + if !info.PreMissing || info.PostMissing { + t.Errorf( + "Expected asymmetric rules for section '%s' to be PreMissing: true, PostMissing: false, got PreMissing: %v, PostMissing: %v", + config.Sections[0].Header.Name, info.PreMissing, info.PostMissing, + ) + }Optional: consider renaming the test to reflect the new model, e.g.,
TestIndexesAsymmetricRules, since UpProperties no longer exists.server/parsers/ini/fields.go (1)
86-98: Add doc comment and keep order guarantee explicitThis method is public and reused across packages. A short doc comment clarifies intent and ordering (iterator yields ascending line order).
Apply this small addition:
- func (s *Section) FindPropertiesByName(name string) []*Property { +// FindPropertiesByName returns all properties in this section whose key name equals `name`. +// The returned slice is ordered by line number (ascending). +func (s *Section) FindPropertiesByName(name string) []*Property {server/handlers/wireguard/lsp/text-document-range-formatting.go (1)
3-9: Import fmt for error constructionIf the document isn’t found, we should return an error. Add fmt to imports.
-import ( - wireguard "config-lsp/handlers/wireguard" - "config-lsp/handlers/wireguard/handlers" - - "github.com/tliron/glsp" - protocol "github.com/tliron/glsp/protocol_3_16" -) +import ( + "fmt" + wireguard "config-lsp/handlers/wireguard" + "config-lsp/handlers/wireguard/handlers" + + "github.com/tliron/glsp" + protocol "github.com/tliron/glsp/protocol_3_16" +)server/handlers/wireguard/analyzer/properties.go (1)
55-62: Prefer normalized header names to avoid case-sensitivity or typosYou already compute
normalizedHeaderName(Line 20). Using it here avoids relying on exact-case string literals "Interface"/"Peer".Example refactor:
- Switch on
normalizedHeaderNameand compare with normalized constants (e.g., fields.InterfaceHeaderNameNormalized, fields.PeerHeaderNameNormalized) if available, or usefields.CreateNormalizedName("Interface")inline.server/handlers/wireguard/lsp/workspace-execute-command.go (1)
34-36: Add alias for the renamed command to avoid client breakageGiven the rename, some clients may send "generateDownRule" while others still use the old string value. Support both to be safe.
case string(handlers.CodeActionGenerateDownRule): args := handlers.CodeActionGenerateDownRuleArgsFromArguments(params.Arguments[0].(map[string]any)) d := wireguard.DocumentParserMap[args.URI] return args.RunCommand(d) + case "generateDownRule": + args := handlers.CodeActionGenerateDownRuleArgsFromArguments(params.Arguments[0].(map[string]any)) + d := wireguard.DocumentParserMap[args.URI] + return args.RunCommand(d)Note: The constant’s string value appears to still be "generatePostDown"; verify the advertised command list in the root handler matches what clients send.
server/root-handler/handler.go (1)
44-49: Avoid shadowing the imported server package nameThe local variable server shadows the imported package alias, which can be confusing. Rename the variable for clarity.
Apply this diff:
- server := server.NewServer(&lspHandler, lsName, false) + lspSrv := server.NewServer(&lspHandler, lsName, false) common.Log.Info("config-lsp server started") - server.RunStdio() + lspSrv.RunStdio()server/handlers/wireguard/indexes/indexes.go (1)
24-26: *Map keyed by ini.Section: confirm lifecycle stabilityKeying AsymmetricRules by section pointers is fine within a single parse, but across reparses the pointers will change. If this map is cached beyond a single indexing pass, it may not match new sections. Consider deriving a stable key (e.g., section start line or a section ID) if you need cross-parse correlation.
Would you like a follow-up patch to use section start line (uint32) as the map key to improve stability across updates?
server/parsers/ini/formatting.go (1)
25-33: Quote escaping may be heavier than necessaryYou escape both double and single quotes, then wrap the value via propertyTemplate. If the template always uses single quotes (as the pattern suggests), escaping double quotes is unnecessary noise in the formatted output. Prefer escaping only the delimiter used by the template, or adapt the template to choose the delimiter that minimizes escaping.
Would you like me to submit a small refactor that:
- Detects the preferred delimiter based on Value.Raw (if quoted), and
- Escapes only that delimiter before wrapping?
server/parsers/ini/parser_test.go (1)
372-394: Adds coverage for escaped quotes inside quoted valuesGood edge-case coverage including Separator presence. Consider adding a counterpart with single-quoted input to exercise quote normalization symmetry.
Do you want me to add a test for: hello = 'world "escaped"' asserting Value == world "escaped" and Raw == ''world "escaped"''?
server/handlers/wireguard/handlers/formatting.go (1)
10-32: Honor the requested formatting rangeTextDocumentRangeFormatting should limit edits to the requested range. Currently, the entire document is formatted regardless. Filter properties to those that intersect textRange.
Apply this diff, and add the helper below:
for _, section := range d.Config.Sections { it := section.Properties.Iterator() for it.Next() { - prop, ok := it.Value().(*ini.Property) + prop, ok := it.Value().(*ini.Property) if !ok || prop == nil { continue } + // If a non-empty range is provided, only format intersecting properties. + if !(textRange.Start.Line == 0 && textRange.Start.Character == 0 && + textRange.End.Line == 0 && textRange.End.Character == 0) { + if !rangesIntersect(prop.ToLSPRange(), textRange) { + continue + } + } newEdits, err := ini.FormatProperty(prop, options) if err != nil { return nil, err } edits = append(edits, newEdits...) } }Add these helpers in the same file (outside the function):
// compares positions a <= b func posLE(a, b protocol.Position) bool { if a.Line < b.Line { return true } if a.Line > b.Line { return false } return a.Character <= b.Character } // compares positions a < b func posLT(a, b protocol.Position) bool { if a.Line < b.Line { return true } if a.Line > b.Line { return false } return a.Character < b.Character } // intersects if a.Start <= b.End && b.Start <= a.End func rangesIntersect(a, b protocol.Range) bool { return posLE(a.Start, b.End) && posLE(b.Start, a.End) }server/parsers/ini/parser.go (2)
250-251: Double-check property range end after quote trimmingpropertyEnd is set to the trimmed valueEnd. For quoted values, this excludes the closing quote from the Property’s LocationRange.End. If downstream formatting replaces [Start:End) and also injects quotes, you could end up with duplicated or dangling quotes depending on how edits are applied.
- If that exclusion is intentional (e.g., you want to keep the original closing quote in place), all good.
- If the intent is to replace the full property value including quotes, consider setting propertyEnd to rawEnd instead.
Please verify the expected edit semantics end-to-end (apply edits to a buffer). If the current behavior is intended, consider adding a short comment explaining why propertyEnd uses the trimmed end.
Also applies to: 270-279
263-265: Normalization is applied to unquoted values as well — confirm this is intendedNormalizeEscapedQuotes removes backslashes before both double and single quotes. For unquoted values that legitimately contain escaped quotes, this will alter the text. If that’s not desired, consider only normalizing when the value is quoted (you already detect this above).
-Value: utils.NormalizeEscapedQuotes(line[valueStart:valueEnd]), +// Only normalize if originally quoted +quoted := (line[rawStart] == '"' && line[rawEnd-1] == '"') || (line[rawStart] == '\'' && line[rawEnd-1] == '\'') +val := line[valueStart:valueEnd] +if quoted { + val = utils.NormalizeEscapedQuotes(val) +} +Value: val,server/handlers/wireguard/handlers/formatting_test.go (2)
40-53: FormatDocument ignores the provided range — add a test to enforce range-formatting semanticsYour tests pass a non-empty range, but FormatDocument currently formats every property in the document. Consider adding a test with multiple properties where the range includes only one of them, and assert that only properties within the range are edited.
13-61: Reduce duplication with a table-driven test helperThe three tests mostly differ by input/expected edit and a few constants. A table-driven test would reduce duplication and make it easier to add cases (including the range-only case mentioned above).
Also applies to: 64-113, 115-164
server/handlers/wireguard/analyzer/property.go (2)
63-79: Handle the opposite asymmetry: PreDown without PreUpWhen PreMissing is true but there are no PreUp properties, the loop continues with a TODO, so we emit no diagnostic even though an asymmetry exists (e.g., only PreDown is present). Emit the complementary message in this case.
- if info.PreMissing { - properties := section.FindPropertiesByName("PreUp") - - if len(properties) == 0 { - // TODO: Fix later - continue - } - - lastProperty := properties[len(properties)-1] - - ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ - Message: "PreUp is set, but PreDown is not. It is recommended to set both properties symmetrically", - Range: lastProperty.Key.ToLSPRange(), - Severity: &common.SeverityHint, - }) - } + if info.PreMissing { + preUps := section.FindPropertiesByName("PreUp") + if len(preUps) > 0 { + last := preUps[len(preUps)-1] + ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ + Message: "PreUp is set, but PreDown is not. It is recommended to set both properties symmetrically", + Range: last.Key.ToLSPRange(), + Severity: &common.SeverityHint, + }) + } else { + preDowns := section.FindPropertiesByName("PreDown") + if len(preDowns) > 0 { + last := preDowns[len(preDowns)-1] + ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ + Message: "PreDown is set, but PreUp is not. It is recommended to set both properties symmetrically", + Range: last.Key.ToLSPRange(), + Severity: &common.SeverityHint, + }) + } + } + }
81-96: Ditto for PostDown without PostUpMirror the Pre* change for Post* asymmetry so both directions are covered.
- if info.PostMissing { - properties := section.FindPropertiesByName("PostUp") - - if len(properties) == 0 { - // TODO: Fix later - continue - } - - lastProperty := properties[len(properties)-1] - - ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ - Message: "PostUp is set, but PostDown is not. It is recommended to set both properties symmetrically", - Range: lastProperty.Key.ToLSPRange(), - Severity: &common.SeverityHint, - }) - } + if info.PostMissing { + postUps := section.FindPropertiesByName("PostUp") + if len(postUps) > 0 { + last := postUps[len(postUps)-1] + ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ + Message: "PostUp is set, but PostDown is not. It is recommended to set both properties symmetrically", + Range: last.Key.ToLSPRange(), + Severity: &common.SeverityHint, + }) + } else { + postDowns := section.FindPropertiesByName("PostDown") + if len(postDowns) > 0 { + last := postDowns[len(postDowns)-1] + ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ + Message: "PostDown is set, but PostUp is not. It is recommended to set both properties symmetrically", + Range: last.Key.ToLSPRange(), + Severity: &common.SeverityHint, + }) + } + } + }server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go (2)
51-52: Use explicit string conversion when concatenating command IDIf CodeActionGenerateDownRule is a named string type, concatenation with a string can be ambiguous. Convert it to string at the concatenation site and avoid the redundant cast at assignment.
- commandID := "wireguard." + CodeActionGenerateDownRule + commandID := "wireguard." + string(CodeActionGenerateDownRule) ... - Command: string(commandID), + Command: commandID,
15-18: Function name no longer matches behaviorThe function now generates either PreDown or PostDown code actions. Consider renaming to GetGenerateDownRuleCodeActions for clarity (public API impact permitting).
server/handlers/wireguard/indexes/indexes_handlers.go (1)
10-13: Avoid re-normalizing names in tight loopsYou already compute normalized names for PreUp/PostUp/PreDown/PostDown at package scope. In the loop you still call fields.CreateNormalizedName for every property. You can compare against the four precomputed normalized values by normalizing once per property, which you already do — good. If CreateNormalizedName is expensive, consider memoizing arbitrary names elsewhere, but this is minor.
server/handlers/wireguard/handlers/code-actions_generate-down.go (5)
18-23: Guard against invalid command arguments to avoid panicsDirect type assertions on
arguments["URI"]andarguments["Line"]can panic if a client sends unexpected types. Consider validating and returning an error instead.Apply this diff to make parsing safer (call sites will need to handle the error):
-func CodeActionGenerateDownRuleArgsFromArguments(arguments map[string]any) CodeActionGenerateDownRuleArgs { - return CodeActionGenerateDownRuleArgs{ - URI: arguments["URI"].(protocol.DocumentUri), - Line: uint32(arguments["Line"].(float64)), - } -} +func CodeActionGenerateDownRuleArgsFromArguments(arguments map[string]any) (CodeActionGenerateDownRuleArgs, error) { + uri, ok := arguments["URI"].(protocol.DocumentUri) + if !ok { + return CodeActionGenerateDownRuleArgs{}, fmt.Errorf("missing or invalid 'URI' argument") + } + lineFloat, ok := arguments["Line"].(float64) + if !ok { + return CodeActionGenerateDownRuleArgs{}, fmt.Errorf("missing or invalid 'Line' argument") + } + return CodeActionGenerateDownRuleArgs{ + URI: uri, + Line: uint32(lineFloat), + }, nil +}
30-35: Clarify error for missing section vs. propertyHelpful for users and logs if you differentiate which entity was not found.
- if section == nil || property == nil { - return nil, errors.New("No section or property found at the specified line") - } + if section == nil { + return nil, errors.New("No section found at the specified line") + } + if property == nil { + return nil, errors.New("No property found at the specified line") + }
37-43: Be robust to whitespace/trailing semicolons and surface parse errors
- Splitting on “;” without trimming yields empty tokens on trailing semicolons.
generateInvertedRulescurrently returns nil on error, making it hard to differentiate empty vs. invalid input.Trim and filter empties, and consider returning an error from
generateInvertedRulesso the user gets actionable diagnostics.- rules := strings.Split(property.Value.Value, ";") - invertedRules := generateInvertedRules(rules) - - if len(invertedRules) == 0 { - return nil, errors.New("No valid iptables rules found to invert") - } + raw := property.Value.Value + parts := strings.Split(raw, ";") + var rules []string + for _, p := range parts { + if r := strings.TrimSpace(p); r != "" { + rules = append(rules, r) + } + } + invertedRules, err := generateInvertedRules(rules) + if err != nil { + return nil, err + } + if len(invertedRules) == 0 { + return nil, errors.New("No valid iptables rules found to invert") + }Note: If semicolons inside quotes are a realistic possibility in your values, consider a quote-aware splitter in utils (e.g., a small state machine over the raw value) to avoid breaking on literal semicolons.
54-54: Prefer INI formatter over hand-crafted property stringTo preserve house style (spacing, quoting, normalization), build the property using the INI formatter introduced in server/parsers/ini/formatting.go rather than
fmt.Sprintf. This will keep formatting consistent across code actions and manual formatting.If
FormatPropertyis available, construct the new property text via that function and prepend a newline only if needed by the formatter or surrounding context.
75-98: Improve error reporting and whitespace normalization in rule inversion
- Return an error instead of
nilto disambiguate “no rules” from “invalid rules”.- Normalize whitespace with
strings.Fieldsto collapse arbitrary runs of spaces (safer than a single ReplaceAll of double spaces).-func generateInvertedRules(rules []string) []string { - var postDownRules []string +func generateInvertedRules(rules []string) ([]string, error) { + var inverted []string for _, rule := range rules { ipTableRule, err := utils.ParseIpTableRule(rule) if err != nil { - return nil + return nil, fmt.Errorf("invalid iptables rule %q: %w", rule, err) } inverted := ipTableRule.InvertAction() if inverted.Action != utils.IpTableActionDelete { // We only want to generate actions for common actions - return nil + return nil, fmt.Errorf("unsupported inverted action %q; expected delete", inverted.Action) } - ruleStringRaw := inverted.String() - ruleString := strings.ReplaceAll(strings.TrimSpace(ruleStringRaw), " ", " ") - postDownRules = append(postDownRules, ruleString) + ruleStringRaw := inverted.String() + ruleString := strings.Join(strings.Fields(strings.TrimSpace(ruleStringRaw)), " ") + inverted = append(inverted, ruleString) } - return postDownRules + return inverted, nil }And ensure the caller handles the new error (see earlier suggestion for lines 37-43).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (21)
server/handlers/wireguard/analyzer/properties.go(1 hunks)server/handlers/wireguard/analyzer/property.go(1 hunks)server/handlers/wireguard/handlers/code-actions.go(1 hunks)server/handlers/wireguard/handlers/code-actions_generate-down.go(1 hunks)server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go(2 hunks)server/handlers/wireguard/handlers/formatting.go(1 hunks)server/handlers/wireguard/handlers/formatting_test.go(1 hunks)server/handlers/wireguard/indexes/indexes.go(1 hunks)server/handlers/wireguard/indexes/indexes_handlers.go(2 hunks)server/handlers/wireguard/indexes/indexes_test.go(1 hunks)server/handlers/wireguard/lsp/text-document-range-formatting.go(1 hunks)server/handlers/wireguard/lsp/workspace-execute-command.go(1 hunks)server/parsers/ini/ast.go(1 hunks)server/parsers/ini/fields.go(1 hunks)server/parsers/ini/formatting.go(1 hunks)server/parsers/ini/parser.go(1 hunks)server/parsers/ini/parser_test.go(1 hunks)server/root-handler/handler.go(1 hunks)server/root-handler/lsp/text-document-range-formatting.go(2 hunks)server/utils/quotes.go(1 hunks)server/utils/quotes_test.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (18)
server/parsers/ini/fields.go (1)
server/parsers/ini/ast.go (2)
Section(44-50)Property(27-34)
server/handlers/wireguard/lsp/text-document-range-formatting.go (1)
server/handlers/wireguard/handlers/formatting.go (1)
FormatDocument(10-32)
server/root-handler/lsp/text-document-range-formatting.go (1)
server/handlers/wireguard/lsp/text-document-range-formatting.go (1)
TextDocumentRangeFormatting(11-22)
server/handlers/wireguard/handlers/formatting.go (3)
server/handlers/wireguard/shared.go (1)
WGDocument(9-12)server/parsers/ini/ast.go (2)
Config(53-59)Property(27-34)server/parsers/ini/formatting.go (1)
FormatProperty(14-44)
server/parsers/ini/formatting.go (2)
server/common/formatting/formatting.go (1)
FormatTemplate(20-20)server/parsers/ini/ast.go (1)
Property(27-34)
server/handlers/wireguard/indexes/indexes.go (1)
server/parsers/ini/ast.go (1)
Section(44-50)
server/handlers/wireguard/analyzer/properties.go (3)
server/handlers/wireguard/fields/common.go (1)
NormalizedName(5-5)server/parsers/ini/ast.go (1)
Header(37-41)server/handlers/wireguard/fields/fields.go (2)
InterfaceAllowedDuplicateFields(223-228)PeerAllowedDuplicateFields(318-320)
server/parsers/ini/parser_test.go (2)
server/parsers/ini/parser.go (1)
NewConfig(15-20)server/parsers/ini/ast.go (2)
INIParseConfig(61-64)Property(27-34)
server/utils/quotes_test.go (1)
server/utils/quotes.go (1)
NormalizeEscapedQuotes(101-111)
server/handlers/wireguard/lsp/workspace-execute-command.go (2)
server/handlers/wireguard/handlers/code-actions.go (1)
CodeActionGenerateDownRule(15-15)server/handlers/wireguard/handlers/code-actions_generate-down.go (1)
CodeActionGenerateDownRuleArgsFromArguments(18-23)
server/handlers/wireguard/analyzer/property.go (2)
server/handlers/wireguard/indexes/indexes.go (1)
AsymmetricRules(12-15)server/common/lsp.go (1)
SeverityHint(26-26)
server/handlers/wireguard/indexes/indexes_test.go (2)
server/handlers/wireguard/indexes/indexes.go (1)
AsymmetricRules(12-15)server/parsers/ini/ast.go (1)
Header(37-41)
server/handlers/wireguard/handlers/formatting_test.go (5)
server/handlers/wireguard/ast/wireguard.go (1)
NewWGConfig(13-17)server/handlers/wireguard/shared.go (1)
WGDocument(9-12)server/parsers/ini/ast.go (1)
Config(53-59)server/handlers/wireguard/handlers/formatting.go (1)
FormatDocument(10-32)server/common/location.go (1)
Position(194-196)
server/root-handler/handler.go (1)
server/handlers/wireguard/handlers/code-actions.go (1)
CodeActionGenerateDownRule(15-15)
server/handlers/wireguard/handlers/code-actions_generate-down.go (3)
server/handlers/wireguard/shared.go (1)
WGDocument(9-12)server/utils/time.go (1)
BlockUntilIndexesNotNil(30-32)server/parsers/ini/ast.go (1)
Config(53-59)
server/parsers/ini/parser.go (3)
server/parsers/ini/ast.go (1)
PropertyValue(15-19)server/common/location.go (2)
LocationRange(31-34)Location(10-13)server/utils/quotes.go (1)
NormalizeEscapedQuotes(101-111)
server/handlers/wireguard/indexes/indexes_handlers.go (3)
server/handlers/wireguard/ast/wireguard.go (1)
WGConfig(8-10)server/handlers/wireguard/indexes/indexes.go (3)
WGIndexes(17-26)WGIndexPropertyInfo(7-10)AsymmetricRules(12-15)server/parsers/ini/ast.go (2)
Section(44-50)Property(27-34)
server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go (5)
server/handlers/wireguard/shared.go (1)
WGDocument(9-12)server/handlers/wireguard/handlers/code-actions.go (2)
CodeAction(18-20)CodeActionGenerateDownRule(15-15)server/utils/time.go (1)
BlockUntilIndexesNotNil(30-32)server/handlers/wireguard/indexes/indexes.go (1)
AsymmetricRules(12-15)server/handlers/wireguard/handlers/code-actions_generate-down.go (1)
CodeActionGenerateDownRuleArgs(13-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (11)
server/root-handler/lsp/text-document-range-formatting.go (1)
8-8: DocumentParserMap is populated on TextDocumentDidOpen, so TextDocumentRangeFormatting will always receive a non-nil parser. No further action required.server/root-handler/handler.go (1)
65-66: Command rename wired correctlyThe switch to CodeActionGenerateDownRule in the command list is consistent with the public rename. Good migration.
server/handlers/wireguard/indexes/indexes.go (1)
12-15: Clear per-section asymmetry modelThe AsymmetricRules type is concise and makes intent obvious (missing pre/post). Good foundation for downstream diagnostics.
server/parsers/ini/formatting.go (2)
20-23: Early return for incomplete properties is correctSkipping edits when the key is empty or the value is nil avoids clobbering partial user input during typing.
38-41: Good: single edit per property using precise rangeUsing property.ToLSPRange() ensures edits replace exactly the property line. This is friendly to editors and reduces conflict risk.
server/parsers/ini/parser_test.go (1)
348-370: Solid coverage for quoted valuesThe test validates both normalized Value and preserved Raw for a simple quoted case. Clear assertions and error messages.
server/parsers/ini/parser.go (1)
243-248: Quoted-value trimming + Raw capture look solidUsing rawStart/rawEnd to preserve the original substring (with quotes) while trimming for Value is correct and aligns with NormalizeEscapedQuotes. Good separation of concerns between Value and Raw for downstream formatting.
Also applies to: 263-265
server/handlers/wireguard/indexes/indexes_handlers.go (1)
31-49: Asymmetry detection is concise and clearThe pre/post counters with normalized names are a neat approach to identify per-section asymmetry. The resulting AsymmetricRules flags are easy to consume downstream.
Also applies to: 52-55
server/handlers/wireguard/handlers/code-actions_generate-down.go (3)
7-7: fmt import is appropriateRequired for formatted strings used later. No concerns.
13-16: Args struct is clear and minimalFields are well-scoped for the command’s needs (URI + Line). Looks good.
25-28: Indexes readiness check is appropriateEnsures indexes are available before proceeding. This prevents nil derefs downstream.
server/handlers/wireguard/handlers/code-actions_generate-down.go
Outdated
Show resolved
Hide resolved
server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go (1)
19-19: Good: LSP line converted to uint32 to match treemap key typeConverting params.Range.Start.Line to uint32 prevents the treemap comparator panic and keeps the args consistent downstream.
server/handlers/wireguard/handlers/code-actions_generate-down.go (2)
47-53: Good: explicit default case prevents invalid key mappingsFailing fast on unsupported keys avoids generating incorrect insertions and unclear labels.
67-69: Good: insert at end-of-line to preserve inline commentsUsing property.ToLSPRange().End avoids hijacking trailing inline comments on the source line.
🧹 Nitpick comments (2)
server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go (1)
51-56: Cast CodeActionGenerateDownRule to string when composing the command IDMake the string conversion explicit and drop the redundant cast at use-site. This is safer if CodeActionName is a defined type (not an alias).
- commandID := "wireguard." + CodeActionGenerateDownRule + commandID := "wireguard." + string(CodeActionGenerateDownRule) @@ - Command: string(commandID), + Command: commandID,server/handlers/wireguard/handlers/code-actions_generate-down.go (1)
81-98: Be resilient to whitespace/empty fragments and partially valid inputsCurrent logic aborts on the first parse error or non-delete action, which is brittle for inputs like trailing semicolons or mixed valid/invalid rules. Trim, skip empties, and continue on per-fragment errors; the caller already errors if none are valid.
- for _, rule := range rules { - ipTableRule, err := utils.ParseIpTableRule(rule) - - if err != nil { - return nil - } - - inverted := ipTableRule.InvertAction() - - if inverted.Action != utils.IpTableActionDelete { - // We only want to generate actions for common actions - return nil - } + for _, rule := range rules { + r := strings.TrimSpace(rule) + if r == "" { + continue + } + ipTableRule, err := utils.ParseIpTableRule(r) + if err != nil { + continue + } + inverted := ipTableRule.InvertAction() + if inverted.Action != utils.IpTableActionDelete { + // Only generate actions for common actions + continue + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
server/handlers/wireguard/handlers/code-actions.go(1 hunks)server/handlers/wireguard/handlers/code-actions_generate-down.go(1 hunks)server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go(1 hunks)server/handlers/wireguard/lsp/text-document-code-action.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/handlers/wireguard/handlers/code-actions.go
🧰 Additional context used
🧬 Code Graph Analysis (3)
server/handlers/wireguard/lsp/text-document-code-action.go (1)
server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go (1)
GetGenerateDownRuleCodeActions(15-74)
server/handlers/wireguard/handlers/code-actions_generate-down.go (3)
server/handlers/wireguard/shared.go (1)
WGDocument(9-12)server/utils/time.go (1)
BlockUntilIndexesNotNil(30-32)server/parsers/ini/ast.go (1)
Config(53-59)
server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go (6)
server/handlers/wireguard/shared.go (1)
WGDocument(9-12)server/handlers/wireguard/handlers/code-actions.go (2)
CodeAction(18-20)CodeActionGenerateDownRule(15-15)server/utils/time.go (1)
BlockUntilIndexesNotNil(30-32)server/parsers/ini/ast.go (2)
Config(53-59)Property(27-34)server/handlers/wireguard/indexes/indexes.go (1)
AsymmetricRules(12-15)server/handlers/wireguard/handlers/code-actions_generate-down.go (1)
CodeActionGenerateDownRuleArgs(13-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
server/handlers/wireguard/lsp/text-document-code-action.go (1)
20-20: Good switch to GenerateDownRule action sourceThe new action source aligns with the renamed command and per-line generation workflow.
server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
server/handlers/wireguard/handlers/code-actions_generate-down.go (3)
37-39: Resolved: safeguard against nil/empty valueThis addresses the earlier concern about potential panics when splitting an empty value; the error message is clear and contextual.
50-57: Resolved: default branch prevents invalid insertionsThe default case cleanly rejects unsupported keys and addresses the prior “empty newKeyName” issue.
69-72: Resolved: insert at end-of-line avoids hijacking inline commentsSwitching to property.ToLSPRange().End fixes the “inline comment shift” risk. Good catch.
🧹 Nitpick comments (5)
server/handlers/wireguard/handlers/code-actions_generate-down.go (5)
18-23: Defensive decode of command arguments to avoid panicsDirect type assertions on arguments risk panics if keys are missing or types differ (e.g., URI as string). Consider validating types and returning a clear error. This is a breaking change (call sites must handle the error), but it hardens the server against malformed client requests.
-func CodeActionGenerateDownRuleArgsFromArguments(arguments map[string]any) CodeActionGenerateDownRuleArgs { - return CodeActionGenerateDownRuleArgs{ - URI: arguments["URI"].(protocol.DocumentUri), - Line: uint32(arguments["Line"].(float64)), - } -} +func CodeActionGenerateDownRuleArgsFromArguments(arguments map[string]any) (CodeActionGenerateDownRuleArgs, error) { + var uri protocol.DocumentUri + uriAny, ok := arguments["URI"] + if !ok { + return CodeActionGenerateDownRuleArgs{}, fmt.Errorf("missing argument: URI") + } + switch v := uriAny.(type) { + case protocol.DocumentUri: + uri = v + case string: + uri = protocol.DocumentUri(v) + default: + return CodeActionGenerateDownRuleArgs{}, fmt.Errorf("invalid type for URI: %T", uriAny) + } + + lineAny, ok := arguments["Line"] + if !ok { + return CodeActionGenerateDownRuleArgs{}, fmt.Errorf("missing argument: Line") + } + var line uint32 + switch v := lineAny.(type) { + case float64: + line = uint32(v) + case uint32: + line = v + default: + return CodeActionGenerateDownRuleArgs{}, fmt.Errorf("invalid type for Line: %T", lineAny) + } + + return CodeActionGenerateDownRuleArgs{ + URI: uri, + Line: line, + }, nil +}
41-42: Don’t abort on first invalid/mixed rule; trim empties and normalize whitespaceCurrent logic returns nil on the first parse error or on a non-delete inversion, which discards any valid rules present. Prefer skipping invalid/unsupported entries, trimming whitespace, and collapsing multiple spaces so you still generate down rules when possible. If none are valid, the existing len check will report "No valid iptables rules found to invert".
func generateInvertedRules(rules []string) []string { - var postDownRules []string - - for _, rule := range rules { - ipTableRule, err := utils.ParseIpTableRule(rule) - - if err != nil { - return nil - } - - inverted := ipTableRule.InvertAction() - - if inverted.Action != utils.IpTableActionDelete { - // We only want to generate actions for common actions - return nil - } - - ruleStringRaw := inverted.String() - ruleString := strings.ReplaceAll(strings.TrimSpace(ruleStringRaw), " ", " ") - postDownRules = append(postDownRules, ruleString) - } - - return postDownRules + var postDownRules []string + for _, rule := range rules { + // Trim and skip empty segments (e.g., trailing semicolons or accidental double semicolons) + rule = strings.TrimSpace(rule) + if rule == "" { + continue + } + + ipTableRule, err := utils.ParseIpTableRule(rule) + if err != nil { + // Skip unparseable entries instead of aborting everything + continue + } + + inverted := ipTableRule.InvertAction() + if inverted.Action != utils.IpTableActionDelete { + // Only generate supported inverted actions; skip others + continue + } + + // Normalize whitespace robustly + ruleStringRaw := inverted.String() + ruleString := strings.Join(strings.Fields(ruleStringRaw), " ") + postDownRules = append(postDownRules, ruleString) + } + return postDownRules }Also applies to: 81-104
41-42: Normalize escaped quotes before splitting, to avoid splitting inside quoted contentIf the value may contain escaped quotes, normalize first so splitting behaves consistently.
- rules := strings.Split(property.Value.Value, ";") + value := utils.NormalizeEscapedQuotes(property.Value.Value) + rules := strings.Split(value, ";")
58-61: Prevent duplicate Down-key creation in the same sectionIf a PreDown/PostDown already exists in the target section, generating another one can create duplicates. Fail fast.
default: return nil, fmt.Errorf("unsupported key %q at line %d; only PreUp/PostUp are supported", property.Key.Name, args.Line) } + // Avoid creating a duplicate down-key in the same section + if len(section.FindPropertiesByName(newKeyName)) > 0 { + return nil, fmt.Errorf("%s already exists in this section", newKeyName) + } + newRulesString := strings.Join(invertedRules, "; ") newPropertyString := fmt.Sprintf("\n%s = %s", newKeyName, newRulesString)
59-61: Consider using the INI formatter for consistent styleTo keep spacing/quoting consistent with the rest of the document (and user-configured formatting options), consider constructing a Property node and delegating to parsers/ini/formatting.FormatProperty instead of building the string manually. If that’s overkill here, current output is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
server/handlers/wireguard/handlers/code-actions_generate-down.go(1 hunks)server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go(1 hunks)server/handlers/wireguard/handlers/fetch-code-actions_typos.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go
🧰 Additional context used
🧬 Code Graph Analysis (2)
server/handlers/wireguard/handlers/fetch-code-actions_typos.go (1)
server/utils/time.go (1)
BlockUntilIndexesNotNil(30-32)
server/handlers/wireguard/handlers/code-actions_generate-down.go (4)
server/handlers/wireguard/shared.go (1)
WGDocument(9-12)server/utils/time.go (1)
BlockUntilIndexesNotNil(30-32)server/parsers/ini/ast.go (1)
Config(53-59)server/utils/iptables.go (2)
ParseIpTableRule(74-100)IpTableActionDelete(14-14)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
server/handlers/wireguard/handlers/code-actions_generate-down.go (1)
30-35: Good early validation of contextChecking both section and property at the target line avoids downstream nil dereferences and gives a clear error to the client.
| if utils.BlockUntilIndexesNotNil(d.Indexes) == false { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Prevent possible nil-pointer dereference on d before checking d.Indexes
Aligning the guard with d.Indexes makes sense, but if d is ever nil, this will panic before the guard executes. Previously passing d avoided that. Add a defensive nil check and use idiomatic negation.
Apply this diff:
- if utils.BlockUntilIndexesNotNil(d.Indexes) == false {
+ if d == nil || !utils.BlockUntilIndexesNotNil(d.Indexes) {
return nil
}To double-check that all call sites guarantee d is non-nil, you can run:
🏁 Script executed:
#!/bin/bash
# Find all call sites of GetPropertyKeywordTypoFixes and show context.
rg -nP --type=go -C3 '\bGetPropertyKeywordTypoFixes\s*\('
# Explicitly look for nil being passed as the first argument.
rg -nP --type=go '\bGetPropertyKeywordTypoFixes\s*\(\s*nil\b' -C2Length of output: 3015
🏁 Script executed:
#!/bin/bash
# Display the first 200 lines of the wireguard LSP code-action handler
sed -n '1,200p' server/handlers/wireguard/lsp/text-document-code-action.goLength of output: 941
🏁 Script executed:
#!/bin/bash
# Display the contents of the typo fixes handler to inspect nil-checks
sed -n '1,200p' server/handlers/wireguard/handlers/fetch-code-actions_typos.goLength of output: 1940
🏁 Script executed:
#!/bin/bash
# Search for the definition of DocumentParserMap in the wireguard package
rg -nP --type=go '\bDocumentParserMap\b' -C5 server/handlers/wireguardLength of output: 9874
Add a nil‐pointer guard in GetPropertyKeywordTypoFixes to avoid panics
The document (d) is retrieved from DocumentParserMap and may be nil (e.g. if a code‐action is requested before the document is opened). Calling d.Indexes when d is nil will panic. Add a defensive nil check and use idiomatic negation:
• File: server/handlers/wireguard/handlers/fetch-code-actions_typos.go
Lines 22–24
- if utils.BlockUntilIndexesNotNil(d.Indexes) == false {
+ if d == nil || !utils.BlockUntilIndexesNotNil(d.Indexes) {
return nil
}This ensures we never dereference d when it’s nil but still skip typo fixes if the indexes aren’t ready.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if utils.BlockUntilIndexesNotNil(d.Indexes) == false { | |
| return nil | |
| } | |
| if d == nil || !utils.BlockUntilIndexesNotNil(d.Indexes) { | |
| return nil | |
| } |
🤖 Prompt for AI Agents
In server/handlers/wireguard/handlers/fetch-code-actions_typos.go around lines
22–24, add a nil check for the retrieved document before accessing its Indexes
to avoid panics; change the current call to first verify d is not nil and use
idiomatic negation so the guard reads like: if d == nil ||
!utils.BlockUntilIndexesNotNil(d.Indexes) { return nil } ensuring we never
dereference d when it can be nil and still skip typo fixes if indexes are not
ready.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
server/handlers/wireguard/handlers/completions_body.go (3)
147-164: Respect the actual cursor column on empty linesHardcoding start=end=0 forces insertion at column 0 and discards indentation or mid-line insertion. Use the current cursor column to improve UX.
Apply this diff:
- if property == nil { - start = 0 - end = 0 + if property == nil { + start = params.Position.Character + end = params.Position.Character
173-175: Either add a snippet cursor placeholder or use plain text formatYou set InsertTextFormatSnippet but the text has no tabstops. Either change to plain text or add a $0 tabstop so the cursor lands after the equals.
Option A (add tabstop):
- insertText := optionName + " = " + insertText := optionName + " = $0"Option B (make it plain text):
- insertFormat := protocol.InsertTextFormatSnippet + insertFormat := protocol.InsertTextFormatPlainText
10-10: Remove debug print and the fmt importUsing the built-in print for ad-hoc logging is noisy and fmt is otherwise unused. Either switch to your logging facility or remove entirely.
Apply this diff:
-import "fmt" +// import "fmt" @@ - print(fmt.Sprintf("Found %d options for section", len(options))) + // fmt.Printf("Found %d options for section\n", len(options)) // consider a proper logger; remove in productionOr, if you don’t want any logging here:
-import "fmt" @@ - print(fmt.Sprintf("Found %d options for section", len(options)))Also applies to: 167-167
server/handlers/wireguard/analyzer/property.go (1)
72-78: Optional: reduce duplication with a small helper.Both Pre* and Post* branches share the same structure. Consider factoring the “emit diagnostics for asymmetric pair” into a helper to keep this function lean. Happy to draft it if you prefer.
Also applies to: 89-95
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
server/handlers/wireguard/analyzer/property.go(1 hunks)server/handlers/wireguard/handlers/completions_body.go(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
server/handlers/wireguard/handlers/completions_body.go (4)
server/parsers/ini/ast.go (1)
Property(27-34)server/doc-values/value-documentation.go (1)
DocumentationValue(10-13)server/utils/slices.go (1)
MapMapToSlice(34-42)server/handlers/wireguard/fields/fields_formatted.go (1)
AllOptionsFormatted(3-23)
server/handlers/wireguard/analyzer/property.go (2)
server/handlers/wireguard/indexes/indexes.go (1)
AsymmetricRules(12-15)server/common/lsp.go (1)
SeverityHint(26-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
server/handlers/wireguard/handlers/completions_body.go (2)
96-99: Key vs. value routing: early return to key completions looks goodDelegating to getKeyCompletions when the cursor is in the key (or on an empty line) is clear and avoids previous boolean flags. Good change.
113-115: getKeyCompletions signature change is appropriatePassing property and params provides the context needed for precise ranges and cursor-aware behavior.
server/handlers/wireguard/analyzer/property.go (1)
63-66: Good pivot to per-section AsymmetricRules.Iterating over ctx.document.Indexes.AsymmetricRules makes the analyzer section-agnostic and aligns with the new indexing model. Using property.Key.ToLSPRange() to target the property name is a good UX choice.
| if info.PreMissing { | ||
| properties := section.FindPropertiesByName("PreUp") | ||
|
|
||
| if len(properties) == 0 { | ||
| // TODO: Fix later | ||
| continue | ||
| } | ||
|
|
||
| for _, property := range properties { | ||
| ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ | ||
| Message: "PreUp is set, but PreDown is not. It is recommended to set both properties symmetrically", | ||
| Range: property.Key.ToLSPRange(), | ||
| Severity: &common.SeverityHint, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Handle both asymmetry directions and avoid early-continue masking other diagnostics.
Current logic only flags “Up present, Down missing” and early-continues if no “Up” properties exist. That has two downsides:
- It misses the reverse case (“Down present, Up missing”).
- The early continue in the Pre* branch skips evaluating PostMissing for the same section.
Refactor to check both directions and remove the early continue. Example patch:
- if info.PreMissing {
- properties := section.FindPropertiesByName("PreUp")
-
- if len(properties) == 0 {
- // TODO: Fix later
- continue
- }
-
- for _, property := range properties {
- ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{
- Message: "PreUp is set, but PreDown is not. It is recommended to set both properties symmetrically",
- Range: property.Key.ToLSPRange(),
- Severity: &common.SeverityHint,
- })
- }
- }
+ if info.PreMissing {
+ preUp := section.FindPropertiesByName("PreUp")
+ preDown := section.FindPropertiesByName("PreDown")
+
+ if len(preUp) > 0 && len(preDown) == 0 {
+ for _, property := range preUp {
+ ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{
+ Message: "PreUp is set, but PreDown is not. It is recommended to set both properties symmetrically",
+ Range: property.Key.ToLSPRange(),
+ Severity: &common.SeverityHint,
+ })
+ }
+ } else if len(preDown) > 0 && len(preUp) == 0 {
+ for _, property := range preDown {
+ ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{
+ Message: "PreDown is set, but PreUp is not. It is recommended to set both properties symmetrically",
+ Range: property.Key.ToLSPRange(),
+ Severity: &common.SeverityHint,
+ })
+ }
+ }
+ }- if info.PostMissing {
- properties := section.FindPropertiesByName("PostUp")
-
- if len(properties) == 0 {
- // TODO: Fix later
- continue
- }
-
- for _, property := range properties {
- ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{
- Message: "PostUp is set, but PostDown is not. It is recommended to set both properties symmetrically",
- Range: property.Key.ToLSPRange(),
- Severity: &common.SeverityHint,
- })
- }
- }
+ if info.PostMissing {
+ postUp := section.FindPropertiesByName("PostUp")
+ postDown := section.FindPropertiesByName("PostDown")
+
+ if len(postUp) > 0 && len(postDown) == 0 {
+ for _, property := range postUp {
+ ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{
+ Message: "PostUp is set, but PostDown is not. It is recommended to set both properties symmetrically",
+ Range: property.Key.ToLSPRange(),
+ Severity: &common.SeverityHint,
+ })
+ }
+ } else if len(postDown) > 0 && len(postUp) == 0 {
+ for _, property := range postDown {
+ ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{
+ Message: "PostDown is set, but PostUp is not. It is recommended to set both properties symmetrically",
+ Range: property.Key.ToLSPRange(),
+ Severity: &common.SeverityHint,
+ })
+ }
+ }
+ }To confirm whether the reverse-direction diagnostics are already covered by tests (and to see if the early-continue might be masking Post* diagnostics), run:
If you want, I can follow up with unit tests for both asymmetry directions (PreDown-only and PostDown-only) and ensure diagnostics are emitted for all relevant properties in each case.
Also applies to: 81-96
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check for tests referencing symmetric property diagnostics:"
fd -t f -g "*_test.go" >/dev/null || true
rg -n -C2 -e 'PreUp is set, but PreDown is not' \
-e 'PostUp is set, but PostDown is not' \
-e 'PreDown is set, but PreUp is not' \
-e 'PostDown is set, but PostUp is not'
echo
echo "Show analyzer implementation around the symmetric check for context:"
rg -n -C3 'analyzeSymmetricPropertiesSet|FindPropertiesByName\("Pre|Post' server/handlers/wireguard/analyzer/property.goLength of output: 2513
server/handlers/wireguard/analyzer/property.go: Expand symmetric‐property diagnostics and remove early-continue
The current implementation only ever reports “Up set, Down missing” and skips the Post* checks whenever there’s no PreUp, so:
- It never warns “Down set, Up missing” for either Pre or Post.
- The
continuein the PreMissing branch prevents PostMissing from running on the same section.
Please refactor both branches (around lines 64–79 and 81–96) to:
• Look up both Up and Down properties
• Emit a diagnostic in either direction (e.g. “PreDown is set, but PreUp is not…”)
• Remove the early continue so PostMissing always executes
Example replacement for the PreMissing block:
- if info.PreMissing {
- properties := section.FindPropertiesByName("PreUp")
-
- if len(properties) == 0 {
- // TODO: Fix later
- continue
- }
-
- for _, property := range properties {
- ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{
- Message: "PreUp is set, but PreDown is not. It is recommended to set both properties symmetrically",
- Range: property.Key.ToLSPRange(),
- Severity: &common.SeverityHint,
- })
- }
- }
+ if info.PreMissing {
+ preUp := section.FindPropertiesByName("PreUp")
+ preDown := section.FindPropertiesByName("PreDown")
+
+ if len(preUp) > 0 && len(preDown) == 0 {
+ for _, prop := range preUp {
+ ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{
+ Message: "PreUp is set, but PreDown is not. It is recommended to set both properties symmetrically",
+ Range: prop.Key.ToLSPRange(),
+ Severity: &common.SeverityHint,
+ })
+ }
+ }
+ if len(preDown) > 0 && len(preUp) == 0 {
+ for _, prop := range preDown {
+ ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{
+ Message: "PreDown is set, but PreUp is not. It is recommended to set both properties symmetrically",
+ Range: prop.Key.ToLSPRange(),
+ Severity: &common.SeverityHint,
+ })
+ }
+ }
+ }And apply the same pattern to the PostMissing block.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if info.PreMissing { | |
| properties := section.FindPropertiesByName("PreUp") | |
| if len(properties) == 0 { | |
| // TODO: Fix later | |
| continue | |
| } | |
| for _, property := range properties { | |
| ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ | |
| Message: "PreUp is set, but PreDown is not. It is recommended to set both properties symmetrically", | |
| Range: property.Key.ToLSPRange(), | |
| Severity: &common.SeverityHint, | |
| }) | |
| } | |
| } | |
| if info.PreMissing { | |
| preUp := section.FindPropertiesByName("PreUp") | |
| preDown := section.FindPropertiesByName("PreDown") | |
| if len(preUp) > 0 && len(preDown) == 0 { | |
| for _, prop := range preUp { | |
| ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ | |
| Message: "PreUp is set, but PreDown is not. It is recommended to set both properties symmetrically", | |
| Range: prop.Key.ToLSPRange(), | |
| Severity: &common.SeverityHint, | |
| }) | |
| } | |
| } | |
| if len(preDown) > 0 && len(preUp) == 0 { | |
| for _, prop := range preDown { | |
| ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ | |
| Message: "PreDown is set, but PreUp is not. It is recommended to set both properties symmetrically", | |
| Range: prop.Key.ToLSPRange(), | |
| Severity: &common.SeverityHint, | |
| }) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In server/handlers/wireguard/analyzer/property.go around lines 64–96, the
PreMissing and PostMissing branches only report when Up is set and Down is
missing and use an early continue that prevents Post checks from running; change
each branch to look up both the Up and Down properties (e.g.
FindPropertiesByName("PreUp"), FindPropertiesByName("PreDown") for the Pre block
and likewise for Post), emit a diagnostic for either direction ("PreUp is set,
but PreDown is not…" and "PreDown is set, but PreUp is not…") by appending to
ctx.diagnostics with the appropriate property's Key.ToLSPRange(), and remove the
early continue so the PostMissing logic always executes; apply the identical
symmetric lookup/diagnostic pattern to the PostMissing block.
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
server/handlers/bitcoin_conf/handlers/fetch-code-actions_generate-rpc-auth.go (1)
21-46: Potential nil dereference onuserProperty.Value/passwordProperty.ValueEven if both properties exist, their
Valuefields can be nil (e.g., “rpcuser=”). UsinguserProperty.Value.Valuein the title will panic. Guard value presence before building the command and action title.- if property != nil && property.Key != nil && (property.Key.Name == "rpcuser" || property.Key.Name == "rpcpassword") && commands.IsPythonAvailable() { + if property != nil && property.Key != nil && (property.Key.Name == "rpcuser" || property.Key.Name == "rpcpassword") && commands.IsPythonAvailable() { _, userProperty := section.FindFirstPropertyByName("rpcuser") _, passwordProperty := section.FindFirstPropertyByName("rpcpassword") - if userProperty != nil && passwordProperty != nil { + if userProperty != nil && userProperty.Value != nil && passwordProperty != nil && passwordProperty.Value != nil { // Generate RPC auth command commandID := "bitcoinconf." + CodeActionGenerateRPCAuth + username := userProperty.Value.Value command := protocol.Command{ - Title: fmt.Sprintf("Generate RPC Auth for '%s'", userProperty.Value.Value), + Title: fmt.Sprintf("Generate RPC Auth for '%s'", username), Command: string(commandID), Arguments: []any{ CodeActionGenerateRPCAuthArgs{ URI: params.TextDocument.URI, UsernameLine: userProperty.Start.Line, PasswordLine: passwordProperty.Start.Line, }, }, } return []protocol.CodeAction{ { - Title: fmt.Sprintf("Generate RPC Auth for '%s'", userProperty.Value.Value), + Title: fmt.Sprintf("Generate RPC Auth for '%s'", username), Command: &command, }, } } }server/parsers/ini/parser.go (1)
241-271: Fix: Property range can end inside the quoted value, making child Value range extend beyond parentWhen the value is quoted, you increment/decrement
valueStart/valueEnd(for Value.Value) but keeprawStart/rawEnd(for Value.LocationRange and Raw). You then setpropertyEnd = valueEnd, which excludes the closing quote. As a result,newProperty.LocationRange.Endcan be less thannewProperty.Value.End, violating containment expectations and breaking range-based consumers.Set
propertyEndtorawEndinstead, so the property range encloses the full raw value (including quotes).Apply this diff:
- propertyEnd = valueEnd + // Ensure the property's range encloses the entire raw value (including quotes) + propertyEnd = rawEndAlso applies to: 276-285
server/handlers/bitcoin_conf/handlers/completions_property.go (1)
118-132: Fix: Off-by-one cursor shift for quoted values in value completions
cursor.ShiftHorizontal(-int(property.Value.Start.Character))uses the raw value start (including the leading quote). Sinceoption.FetchCompletionsreceivesproperty.Value.Value(quotes removed), the cursor offset should skip the leading quote when the value is quoted.Apply this diff:
- if property.Value == nil { - return option.FetchCompletions("", 0), nil - } else { - return option.FetchCompletions( - property.Value.Value, - cursor.ShiftHorizontal(-int(property.Value.Start.Character)), - ), nil - } + if property.Value == nil { + return option.FetchCompletions("", 0), nil + } + // Adjust offset for quoted values: Value.Value excludes quotes, but Start points to Raw (including quotes) + offset := int(property.Value.Start.Character) + raw := property.Value.Raw + if len(raw) >= 2 && ((raw[0] == '"' && raw[len(raw)-1] == '"') || (raw[0] == '\'' && raw[len(raw)-1] == '\'')) { + offset++ // skip leading quote + } + return option.FetchCompletions( + property.Value.Value, + cursor.ShiftHorizontal(-offset), + ), nil
♻️ Duplicate comments (2)
server/handlers/wireguard/analyzer/property.go (1)
63-76: Handle both asymmetry directions for Pre/Post and drop TODOs**This still only flags “Up set, Down missing” and doesn’t emit the reverse (“Down set, Up missing”). Also, the TODOs can be removed by issuing diagnostics for both directions.
Proposed refactor:
- if info.PreMissing { - properties := section.FindPropertiesByName("PreUp") - - // TODO: Fix later - if len(properties) != 0 { - for _, property := range properties { - ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ - Message: "PreUp is set, but PreDown is not. It is recommended to set both properties symmetrically", - Range: property.Key.ToLSPRange(), - Severity: &common.SeverityHint, - }) - } - } - } + if info.PreMissing { + preUp := section.FindPropertiesByName("PreUp") + preDown := section.FindPropertiesByName("PreDown") + if len(preUp) > 0 && len(preDown) == 0 { + for _, prop := range preUp { + ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ + Message: "PreUp is set, but PreDown is not. It is recommended to set both properties symmetrically", + Range: prop.Key.ToLSPRange(), + Severity: &common.SeverityHint, + }) + } + } + if len(preDown) > 0 && len(preUp) == 0 { + for _, prop := range preDown { + ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ + Message: "PreDown is set, but PreUp is not. It is recommended to set both properties symmetrically", + Range: prop.Key.ToLSPRange(), + Severity: &common.SeverityHint, + }) + } + } + } @@ - if info.PostMissing { - properties := section.FindPropertiesByName("PostUp") - - // TODO: Fix later - if len(properties) != 0 { - for _, property := range properties { - ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ - Message: "PostUp is set, but PostDown is not. It is recommended to set both properties symmetrically", - Range: property.Key.ToLSPRange(), - Severity: &common.SeverityHint, - }) - } - } - } + if info.PostMissing { + postUp := section.FindPropertiesByName("PostUp") + postDown := section.FindPropertiesByName("PostDown") + if len(postUp) > 0 && len(postDown) == 0 { + for _, prop := range postUp { + ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ + Message: "PostUp is set, but PostDown is not. It is recommended to set both properties symmetrically", + Range: prop.Key.ToLSPRange(), + Severity: &common.SeverityHint, + }) + } + } + if len(postDown) > 0 && len(postUp) == 0 { + for _, prop := range postDown { + ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ + Message: "PostDown is set, but PostUp is not. It is recommended to set both properties symmetrically", + Range: prop.Key.ToLSPRange(), + Severity: &common.SeverityHint, + }) + } + } + }Also applies to: 79-91
server/handlers/wireguard/handlers/completions_body.go (1)
141-144: Also guardproperty.Keywhen comparing lines; and nice fix on “current key” detectionThe comparison should handle
property != nil && property.Key != nilto avoid nil deref. Also, switching from section header line to the current property line fixes the earlier bug—thanks for addressing that.- if property != nil && iniProperty.Key.Start.Line == property.Key.Start.Line { + if property != nil && property.Key != nil && iniProperty.Key.Start.Line == property.Key.Start.Line { // The user is currently typing the key, thus we should suggest it continue }
🧹 Nitpick comments (1)
server/handlers/wireguard/handlers/hover.go (1)
21-26: Good nil-guard; also consider guardingsection.Headerbefore dereferencingNow that headers can be nil for placeholder sections, accessing
section.Header.Namebelow can panic. Add a quick header nil-check before using it.) (*protocol.Hover, error) { if property.Key == nil { return nil, nil } + if section.Header == nil { + return nil, nil + } + availableOptions, found := fields.OptionsHeaderMap[fields.CreateNormalizedName(section.Header.Name)]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (16)
server/handlers/bitcoin_conf/analyzer/properties.go(1 hunks)server/handlers/bitcoin_conf/handlers/code-actions_generate-rpc-auth.go(1 hunks)server/handlers/bitcoin_conf/handlers/completions_property.go(4 hunks)server/handlers/bitcoin_conf/handlers/fetch-code-actions_generate-rpc-auth.go(1 hunks)server/handlers/bitcoin_conf/handlers/formatting.go(1 hunks)server/handlers/bitcoin_conf/handlers/hover.go(1 hunks)server/handlers/wireguard/analyzer/properties.go(2 hunks)server/handlers/wireguard/analyzer/property.go(1 hunks)server/handlers/wireguard/handlers/completions_body.go(3 hunks)server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go(1 hunks)server/handlers/wireguard/handlers/fetch-code-actions_key-generation.go(1 hunks)server/handlers/wireguard/handlers/hover.go(1 hunks)server/parsers/ini/ast.go(2 hunks)server/parsers/ini/fields.go(2 hunks)server/parsers/ini/formatting.go(1 hunks)server/parsers/ini/parser.go(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- server/parsers/ini/ast.go
- server/parsers/ini/formatting.go
- server/handlers/wireguard/analyzer/properties.go
- server/handlers/wireguard/handlers/fetch-code-actions_generate-down.go
🧰 Additional context used
🧬 Code Graph Analysis (7)
server/handlers/bitcoin_conf/analyzer/properties.go (1)
server/common/lsp.go (1)
SeverityError(23-23)
server/parsers/ini/fields.go (1)
server/parsers/ini/ast.go (2)
Section(44-50)Property(27-34)
server/handlers/bitcoin_conf/handlers/completions_property.go (3)
server/parsers/ini/ast.go (2)
Section(44-50)Property(27-34)server/utils/slices.go (1)
MapMapToSlice(34-42)server/doc-values/value-documentation.go (1)
DocumentationValue(10-13)
server/handlers/wireguard/handlers/completions_body.go (5)
server/parsers/ini/ast.go (1)
Property(27-34)server/handlers/wireguard/fields/common.go (2)
CreateNormalizedName(7-9)NormalizedName(5-5)server/utils/slices.go (1)
MapMapToSlice(34-42)server/doc-values/value-documentation.go (1)
DocumentationValue(10-13)server/handlers/wireguard/fields/fields_formatted.go (1)
AllOptionsFormatted(3-23)
server/handlers/wireguard/analyzer/property.go (2)
server/handlers/wireguard/indexes/indexes.go (1)
AsymmetricRules(12-15)server/common/lsp.go (1)
SeverityHint(26-26)
server/parsers/ini/parser.go (3)
server/parsers/ini/ast.go (2)
PropertyKey(9-12)PropertyValue(15-19)server/common/location.go (2)
LocationRange(31-34)Location(10-13)server/utils/quotes.go (1)
NormalizeEscapedQuotes(101-111)
server/handlers/bitcoin_conf/handlers/formatting.go (1)
server/parsers/ini/ast.go (1)
Property(27-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (13)
server/handlers/wireguard/handlers/completions_body.go (1)
132-135: Good: skip properties with nil Key during duplicate filteringPrevents panics now that Key can be nil. No further action needed.
server/handlers/bitcoin_conf/handlers/hover.go (1)
18-20: Good defensive check: avoid nil deref when Key is missingEarly-returning on
property.Key == nilprevents panics when the parser yields properties without keys. Looks good.server/handlers/wireguard/handlers/fetch-code-actions_key-generation.go (1)
23-24: Solid: includeproperty.Key == nilin guardThis aligns with parser changes making Key optional and prevents crashes in code-action discovery.
server/handlers/bitcoin_conf/handlers/formatting.go (1)
19-21: Defensive nil-guards are appropriate hereSkipping properties without Key or Value avoids panics and aligns with the new INI model where Key can be nil. This is the right trade-off for formatting.
server/handlers/bitcoin_conf/analyzer/properties.go (1)
24-31: Nil-safe missing-name handling looks goodSwitching to
property.Key == nil, usingproperty.ToLSPRange(), and short-circuiting withcontinueprevents nil dereferences downstream and produces a sensible diagnostic range.server/parsers/ini/fields.go (2)
79-82: Solid nil-guard inFindFirstPropertyByNameThe additional
property.Key == nilcheck prevents panics when Keys are absent. This aligns with the parser changes makingKeyoptional.
91-108: New helperFindPropertiesByNameis useful and straightforwardIterating in key order via the treemap iterator and filtering on
Key != nil && Key.Name == nameis correct. Returning pointers keeps it efficient.server/parsers/ini/parser.go (3)
31-31: Regex makes key optional, but value capture skips leading spaces and inline-comment handling is brittleTwo points to double-check:
- The value group
\s*(?P<value>\S.*?)?starts at the first non-space, soProperty.Value.Rawwill exclude any spaces immediately after=. If you intended Raw to reflect the original substring after=(including quotes but not necessarily trimming leading spaces), this deviates. Confirm intent.- Trailing inline comments are recognized only when there is exactly one space before
;/#((?: [;#].*)?). This is fragile (e.g., tabs or multiple spaces will often fold into\s*before, and no-space comments likekey=value;commentwill be treated as part of the value). If you want more robust inline comment stripping while avoiding interference with quoted values, consider tightening with parser-side logic rather than regex, or at least broaden the pattern to allow tabs/multiple spaces.Do you want
Rawto include leading spaces after=? Also, should inline comments with tabs/multiple spaces or no spaces be stripped?
145-171: LGTM: Incomplete property handling builds a proper Key and rangeCreating a Key from the trimmed indices for lines without
=and assigning a precise LocationRange is sound and aligns with downstream usage.
198-219: LGTM: Nil-able Key and propertyStart are correctly derivedUsing a
*PropertyKeyand settingpropertyStartto the key start only when the key exists is a clean way to model keyless properties.server/handlers/bitcoin_conf/handlers/completions_property.go (3)
88-108: LGTM: Snippet-based key insertion with explicit TextEdit rangeGood switch to snippet insertion and explicit range replacement; it plays well with property-aware editing.
52-63: LGTM: Allowed-duplicate filtering is handled correctlySkipping removal when
AllowedDuplicateOptionscontains the key and guarding onproperty.Key == nilprevents accidental deletions.
3-13: Toolchain compatibility confirmedThe
go.modspecifies Go 1.24, which satisfies themaps.Copyrequirement (Go 1.21+). No changes needed.
| if userProperty == nil || passwordProperty == nil || userProperty.Key == nil || passwordProperty.Key == nil || userProperty.Key.Name != "rpcuser" || passwordProperty.Key.Name != "rpcpassword" { | ||
| return nil, fmt.Errorf("rpcuser or rpcpassword not found at specified lines") | ||
| } |
There was a problem hiding this comment.
Also guard against missing values for rpcuser/rpcpassword
You added nil-key checks, but userProperty.Value or passwordProperty.Value can also be nil, leading to a panic when accessing .Value.Value. Treat missing values as invalid input and return the same error.
- if userProperty == nil || passwordProperty == nil || userProperty.Key == nil || passwordProperty.Key == nil || userProperty.Key.Name != "rpcuser" || passwordProperty.Key.Name != "rpcpassword" {
+ if userProperty == nil ||
+ passwordProperty == nil ||
+ userProperty.Key == nil ||
+ passwordProperty.Key == nil ||
+ userProperty.Value == nil ||
+ passwordProperty.Value == nil ||
+ userProperty.Key.Name != "rpcuser" ||
+ passwordProperty.Key.Name != "rpcpassword" {
return nil, fmt.Errorf("rpcuser or rpcpassword not found at specified lines")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if userProperty == nil || passwordProperty == nil || userProperty.Key == nil || passwordProperty.Key == nil || userProperty.Key.Name != "rpcuser" || passwordProperty.Key.Name != "rpcpassword" { | |
| return nil, fmt.Errorf("rpcuser or rpcpassword not found at specified lines") | |
| } | |
| if userProperty == nil || | |
| passwordProperty == nil || | |
| userProperty.Key == nil || | |
| passwordProperty.Key == nil || | |
| userProperty.Value == nil || | |
| passwordProperty.Value == nil || | |
| userProperty.Key.Name != "rpcuser" || | |
| passwordProperty.Key.Name != "rpcpassword" { | |
| return nil, fmt.Errorf("rpcuser or rpcpassword not found at specified lines") | |
| } |
🤖 Prompt for AI Agents
In server/handlers/bitcoin_conf/handlers/code-actions_generate-rpc-auth.go
around lines 34 to 36, the current guard checks Key fields but not the Value
fields, which can be nil and cause a panic when later accessing .Value.Value;
update the conditional to also verify userProperty.Value and
passwordProperty.Value are non-nil and that their inner Value fields are present
(e.g., userProperty.Value != nil && userProperty.Value.Value != nil &&
passwordProperty.Value != nil && passwordProperty.Value.Value != nil) and return
the same fmt.Errorf("rpcuser or rpcpassword not found at specified lines") if
any of those are missing.
| var start uint32 | ||
| var end uint32 | ||
|
|
||
| if property == nil { | ||
| start = 0 | ||
| end = 0 | ||
| } else { | ||
| start = property.Key.Start.Character | ||
|
|
||
| if property.Value != nil { | ||
| end = property.Value.Start.Character | ||
| } else if property.Separator != nil { | ||
| end = property.Separator.End.Character | ||
| } else { | ||
| end = property.Key.End.Character | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Nil-pointer risk and more robust edit range: derive start from property range and guard Key/Separator
start = property.Key.Start.Character panics when property.Key is nil. Also, for keyless properties, using property.Start.Character is safer and aligns with parser’s propertyStart logic.
Apply this diff:
- var start uint32
- var end uint32
-
- if property == nil {
- start = 0
- end = 0
- } else {
- start = property.Key.Start.Character
-
- if property.Value != nil {
- end = property.Value.Start.Character
- } else if property.Separator != nil {
- end = property.Separator.End.Character
- } else {
- end = property.Key.End.Character
- }
- }
+ var start uint32
+ var end uint32
+
+ if property == nil {
+ start, end = 0, 0
+ } else {
+ // Safe start: parser sets property.Start.Character to key start if key exists, else 0
+ start = property.Start.Character
+
+ if property.Value != nil {
+ end = property.Value.Start.Character
+ } else if property.Separator != nil {
+ end = property.Separator.End.Character
+ } else if property.Key != nil {
+ end = property.Key.End.Character
+ } else {
+ end = property.End.Character
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var start uint32 | |
| var end uint32 | |
| if property == nil { | |
| start = 0 | |
| end = 0 | |
| } else { | |
| start = property.Key.Start.Character | |
| if property.Value != nil { | |
| end = property.Value.Start.Character | |
| } else if property.Separator != nil { | |
| end = property.Separator.End.Character | |
| } else { | |
| end = property.Key.End.Character | |
| } | |
| } | |
| var start uint32 | |
| var end uint32 | |
| if property == nil { | |
| start, end = 0, 0 | |
| } else { | |
| // Safe start: parser sets property.Start.Character to key start if key exists, else 0 | |
| start = property.Start.Character | |
| if property.Value != nil { | |
| end = property.Value.Start.Character | |
| } else if property.Separator != nil { | |
| end = property.Separator.End.Character | |
| } else if property.Key != nil { | |
| end = property.Key.End.Character | |
| } else { | |
| end = property.End.Character | |
| } | |
| } |
🤖 Prompt for AI Agents
In server/handlers/bitcoin_conf/handlers/completions_property.go around lines 65
to 82, the current code reads property.Key.Start.Character without guarding Key
which can nil‑panic and also doesn't use the property range for keyless
properties; change logic to derive start from property.Start.Character (fallback
to property.Key.Start.Character only if Start is unavailable), and for end use
this order: if property.Value != nil use property.Value.Start.Character, else if
property.Separator != nil use property.Separator.End.Character, else if
property.Key != nil use property.Key.End.Character, otherwise use
property.End.Character; add nil checks before accessing Key/Separator to prevent
panics.
| if info.PostMissing { | ||
| properties := section.FindPropertiesByName("PostUp") | ||
|
|
||
| // TODO: Fix later | ||
| if len(properties) == 0 { | ||
| for _, property := range properties { | ||
| ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ | ||
| Message: "PostUp is set, but PostDown is not. It is recommended to set both properties symmetrically", | ||
| Range: property.Key.ToLSPRange(), | ||
| Severity: &common.SeverityHint, | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
Bug: PostMissing condition uses == 0 and then iterates; diagnostics never fire
The branch checks len(properties) == 0 and then loops over properties, which will be empty, so no diagnostics are produced even when they should be.
Apply this fix:
- // TODO: Fix later
- if len(properties) == 0 {
- for _, property := range properties {
+ // TODO: Fix later
+ if len(properties) != 0 {
+ for _, property := range properties {
ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{
Message: "PostUp is set, but PostDown is not. It is recommended to set both properties symmetrically",
Range: property.Key.ToLSPRange(),
Severity: &common.SeverityHint,
})
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if info.PostMissing { | |
| properties := section.FindPropertiesByName("PostUp") | |
| // TODO: Fix later | |
| if len(properties) == 0 { | |
| for _, property := range properties { | |
| ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ | |
| Message: "PostUp is set, but PostDown is not. It is recommended to set both properties symmetrically", | |
| Range: property.Key.ToLSPRange(), | |
| Severity: &common.SeverityHint, | |
| }) | |
| } | |
| } | |
| if info.PostMissing { | |
| properties := section.FindPropertiesByName("PostUp") | |
| // TODO: Fix later | |
| if len(properties) != 0 { | |
| for _, property := range properties { | |
| ctx.diagnostics = append(ctx.diagnostics, protocol.Diagnostic{ | |
| Message: "PostUp is set, but PostDown is not. It is recommended to set both properties symmetrically", | |
| Range: property.Key.ToLSPRange(), | |
| Severity: &common.SeverityHint, | |
| }) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In server/handlers/wireguard/analyzer/property.go around lines 79 to 91, the
code checks if len(properties) == 0 and then iterates over properties (which are
empty) so diagnostics never fire; change the condition to if len(properties) > 0
{ and keep the existing loop to append a diagnostic for each found PostUp
property indicating PostDown is missing so the diagnostics are actually
produced.
| var start uint32 | ||
| var end uint32 | ||
|
|
||
| if property == nil { | ||
| start = 0 | ||
| end = 0 | ||
| } else { | ||
| start = property.Key.Start.Character | ||
|
|
||
| if property.Value != nil { | ||
| end = property.Value.Start.Character | ||
| } else if property.Separator != nil { | ||
| end = property.Separator.End.Character | ||
| } else { | ||
| end = property.Key.End.Character | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Compute start/end safely when Key/Separator/Value can be nil
Multiple potential nil-derefs here if property.Key is nil (common with malformed lines). Use property’s own LocationRange as fallback.
- var start uint32
- var end uint32
- if property == nil {
- start = 0
- end = 0
- } else {
- start = property.Key.Start.Character
- if property.Value != nil {
- end = property.Value.Start.Character
- } else if property.Separator != nil {
- end = property.Separator.End.Character
- } else {
- end = property.Key.End.Character
- }
- }
+ var start uint32
+ var end uint32
+ if property == nil {
+ start = 0
+ end = 0
+ } else {
+ if property.Key != nil {
+ start = property.Key.Start.Character
+ } else {
+ start = property.Start.Character
+ }
+ if property.Value != nil {
+ end = property.Value.Start.Character
+ } else if property.Separator != nil {
+ end = property.Separator.End.Character
+ } else if property.Key != nil {
+ end = property.Key.End.Character
+ } else {
+ end = property.End.Character
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| var start uint32 | |
| var end uint32 | |
| if property == nil { | |
| start = 0 | |
| end = 0 | |
| } else { | |
| start = property.Key.Start.Character | |
| if property.Value != nil { | |
| end = property.Value.Start.Character | |
| } else if property.Separator != nil { | |
| end = property.Separator.End.Character | |
| } else { | |
| end = property.Key.End.Character | |
| } | |
| } | |
| var start uint32 | |
| var end uint32 | |
| if property == nil { | |
| start = 0 | |
| end = 0 | |
| } else { | |
| if property.Key != nil { | |
| start = property.Key.Start.Character | |
| } else { | |
| start = property.Start.Character | |
| } | |
| if property.Value != nil { | |
| end = property.Value.Start.Character | |
| } else if property.Separator != nil { | |
| end = property.Separator.End.Character | |
| } else if property.Key != nil { | |
| end = property.Key.End.Character | |
| } else { | |
| end = property.End.Character | |
| } | |
| } |
🤖 Prompt for AI Agents
In server/handlers/wireguard/handlers/completions_body.go around lines 149 to
165, the current logic dereferences property.Key, property.Value and
property.Separator without guarding against nil (leading to panics on malformed
lines); change the logic to compute start/end defensively: if property is nil
set start/end to 0; otherwise if property.Key != nil use its Start.Character for
start, else fall back to property.LocationRange.Start.Character; for end, prefer
property.Value.Start.Character when property.Value != nil, else if
property.Separator != nil use property.Separator.End.Character, else if
property.Key != nil use property.Key.End.Character, else fall back to
property.LocationRange.End.Character; ensure all accesses check for nil before
dereference and keep types uint32.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (7)
server/handlers/wireguard/analyzer/structure.go (2)
44-44: Guard the type assertion to avoid a panic if the container’s type changesIf section.Properties ever contains a different element type, the hard assertion will panic. Prefer a safe cast.
Apply this diff:
- property := it.Value().(*ini.Property) + v := it.Value() + property, ok := v.(*ini.Property) + if !ok || property == nil { + continue + }
46-52: Consider enriching diagnostics with Source/Code for better UX and code actionsAdding Source (e.g., "wireguard-structure") and a stable Code (e.g., "wg.missingPropertyKey") to the Diagnostic helps clients group issues and enables targeted code actions. Implement only if protocol.Diagnostic in your GLSP lib exposes these fields in your version.
If helpful, I can generate a quick script to scan for other diagnostics in this package and suggest consistent Source/Code values.
server/handlers/wireguard/ast/wireguard_handler.go (1)
3-11: Prefer pointer receiver and add light defensive checks (optional).The implementation is correct and straightforward. Two optional improvements:
Use a pointer receiver to avoid copying WGConfig (and for consistency if most methods use pointer receivers).
Add a defensive check in case any section is nil (future-proofing).
Is header name comparison intended to be case-sensitive? If not, consider strings.EqualFold.
Apply this minimal diff for the pointer receiver:
-func (c WGConfig) IncludesHeader(headerName string) bool { +func (c *WGConfig) IncludesHeader(headerName string) bool { for _, section := range c.Sections { - if section.Header.Name == headerName { + if section != nil && section.Header.Name == headerName { return true } } return false }If you decide to make it case-insensitive, also add the strings import and change the comparison:
- if section != nil && section.Header.Name == headerName { + if section != nil && strings.EqualFold(section.Header.Name, headerName) {And add this import outside the shown range:
import "strings"server/handlers/wireguard/handlers/completions.go (1)
3-8: Drop unused fmt import (only used by now-removed debug).Once stdout prints are removed or moved to a logger, the fmt import becomes unused and should be removed.
Apply this diff:
import ( "config-lsp/handlers/wireguard" - "fmt" protocol "github.com/tliron/glsp/protocol_3_16" )server/handlers/wireguard/handlers/completions_header.go (1)
3-10: Drop fmt import if only used for debug print.If you remove the stdout debug line, fmt becomes unused here and should be removed.
Apply this diff:
import ( "config-lsp/handlers/wireguard" "config-lsp/handlers/wireguard/fields" "config-lsp/parsers/ini" - "fmt" protocol "github.com/tliron/glsp/protocol_3_16" )server/parsers/ini/parser_test.go (2)
365-369: Guard against missing property to avoid panics (check the “found” flag).Properties.Get returns a found flag. If it’s false, the subsequent type assertion/dereference will panic and the test won’t produce a helpful failure message.
Apply this diff to make the test more robust:
- rawProperty, _ := config.Sections[0].Properties.Get(uint32(0)) - property := rawProperty.(*Property) + rawProperty, found := config.Sections[0].Properties.Get(uint32(0)) + if !found { + t.Fatalf("Parse: Expected first property at index 0, but none found") + } + property := rawProperty.(*Property)
389-393: Also check the “found” flag here to prevent a panic on type assertion.Same robustness issue as above; make sure you handle the case when the property isn’t present.
- rawProperty, _ := config.Sections[0].Properties.Get(uint32(0)) - property := rawProperty.(*Property) + rawProperty, found := config.Sections[0].Properties.Get(uint32(0)) + if !found { + t.Fatalf("Parse: Expected first property at index 0, but none found") + } + property := rawProperty.(*Property)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
server/handlers/bitcoin_conf/handlers/completions_property.go(4 hunks)server/handlers/wireguard/analyzer/structure.go(2 hunks)server/handlers/wireguard/ast/wireguard_handler.go(1 hunks)server/handlers/wireguard/handlers/completions.go(2 hunks)server/handlers/wireguard/handlers/completions_body.go(3 hunks)server/handlers/wireguard/handlers/completions_header.go(2 hunks)server/parsers/ini/parser_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- server/handlers/bitcoin_conf/handlers/completions_property.go
- server/handlers/wireguard/handlers/completions_body.go
🧰 Additional context used
🧬 Code Graph Analysis (4)
server/handlers/wireguard/ast/wireguard_handler.go (2)
server/handlers/wireguard/ast/wireguard.go (1)
WGConfig(8-10)server/parsers/ini/ast.go (1)
Header(37-41)
server/handlers/wireguard/handlers/completions_header.go (2)
server/parsers/ini/ast.go (1)
Config(53-59)server/handlers/wireguard/fields/fields.go (2)
HeaderInterfaceEnum(8-11)HeaderPeerEnum(12-19)
server/handlers/wireguard/analyzer/structure.go (2)
server/parsers/ini/ast.go (1)
Property(27-34)server/common/lsp.go (1)
SeverityError(23-23)
server/parsers/ini/parser_test.go (2)
server/parsers/ini/parser.go (1)
NewConfig(15-20)server/parsers/ini/ast.go (2)
INIParseConfig(61-64)Property(27-34)
🔇 Additional comments (3)
server/handlers/wireguard/analyzer/structure.go (2)
6-6: Import looks correct and necessaryThe new ini import is used for the Property type assertion below. No issues.
40-54: Good addition: per-property “missing key” diagnosticsIterating properties when a section isn’t empty and emitting an error when Key == nil is aligned with the updated AST (Property.Key can be nil). Using property.ToLSPRange() is the right choice since there is no key range to point at.
server/handlers/wireguard/handlers/completions_header.go (1)
50-54: Good refactor: replace manual scan with IncludesHeader.Switching to d.Config.IncludesHeader("Interface") improves readability and encapsulates the lookup logic. This also reduces duplication across the codebase.
| rawProperty, _ := config.Sections[0].Properties.Get(uint32(0)) | ||
| property := rawProperty.(*Property) | ||
| if !(property.Key == nil && property.Value != nil && property.Value.Value == "world" && property.Separator != nil) { | ||
| t.Errorf("Parse: Expected property to be 'hello =', but got '%s = %v'", property.Key.Name, property.Value) | ||
| } | ||
| } |
There was a problem hiding this comment.
Fix nil-pointer dereference in error message and correct the expected text.
If the assertion fails (e.g., Key is nil), referencing property.Key.Name in t.Errorf will panic. The message also mentions “hello =” while the sample is “=world”.
Apply this diff:
- rawProperty, _ := config.Sections[0].Properties.Get(uint32(0))
- property := rawProperty.(*Property)
- if !(property.Key == nil && property.Value != nil && property.Value.Value == "world" && property.Separator != nil) {
- t.Errorf("Parse: Expected property to be 'hello =', but got '%s = %v'", property.Key.Name, property.Value)
- }
+ rawProperty, found := config.Sections[0].Properties.Get(uint32(0))
+ if !found {
+ t.Fatalf("Parse: Expected first property at index 0, but none found")
+ }
+ property := rawProperty.(*Property)
+ if !(property.Key == nil && property.Value != nil && property.Value.Value == "world" && property.Separator != nil) {
+ keyName := "<nil>"
+ if property.Key != nil {
+ keyName = property.Key.Name
+ }
+ val := "<nil>"
+ if property.Value != nil {
+ val = property.Value.Value
+ }
+ t.Errorf("Parse: Expected incomplete property '=world' (no key). got key=%s, value=%s, sepPresent=%t", keyName, val, property.Separator != nil)
+ }🤖 Prompt for AI Agents
In server/parsers/ini/parser_test.go around lines 409 to 414, the test error
path dereferences property.Key.Name which can be nil and will panic, and the
expectation text is incorrect ("hello =" vs the sample "=world"); fix by
building a safe key string before logging (e.g. set keyName to "<nil>" and
overwrite if property.Key != nil) and update the error message to reflect the
correct expectation (e.g. expect "=world"), then use that safe keyName in the
t.Errorf call instead of property.Key.Name.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
flake.nix(1 hunks)server/common/common.go(1 hunks)server/handlers/wireguard/handlers/completions.go(1 hunks)server/handlers/wireguard/handlers/completions_header.go(1 hunks)vs-code-extension/package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- vs-code-extension/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- server/handlers/wireguard/handlers/completions_header.go
🧰 Additional context used
🪛 GitHub Actions: Run Go tests
server/handlers/wireguard/handlers/completions.go
[error] 5-5: Go build failed during step: nix develop --command bash -c 'cd server && go build'. Error: 'fmt' imported and not used (handlers/wireguard/handlers/completions.go:5).
🔇 Additional comments (2)
flake.nix (1)
26-26: Consistency Check Passed – All version sources match 0.3.4.flake.nix, server/common/common.go, and vs-code-extension/package.json are all at version 0.3.4. No further updates required.
server/common/common.go (1)
5-5: LGTM: Version updated to 0.3.4 with CI:CD marker intactChange is minimal, consistent with the packaging update in flake.nix.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Chores