Conversation
📝 WalkthroughWalkthroughAdds LSP generation and runtime support: a Python LSP schema code generator, new C++ LSP type definitions, comprehensive spelling/map-key utilities, enum-string serialization support, improved map key parsing, and related serializer/deserializer and reflection helpers. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI
participant Fetcher as SchemaFetcher
participant Parser as SchemaParser
participant Generator as Generator
participant Renderer as TypeRenderer
participant Writer as FileWriter
CLI->>Fetcher: fetch_schema(--fetch-schema / --schema)
Fetcher-->>Parser: raw JSON
Parser->>Generator: SchemaModel
Generator->>Generator: build_name_map(), build_node_dependencies()
Generator->>Generator: topological_order(nodes,deps)
loop for each node
Generator->>Renderer: render_type / render_or
Renderer-->>Generator: C++ type strings
alt struct
Generator->>Generator: emit_struct()
else enum
Generator->>Generator: emit_enum()
else alias
Generator->>Generator: emit_alias()
end
end
Generator->>Writer: make_header(includes, blocks)
Writer->>Writer: write_file(output_path, content)
Writer-->>CLI: summary metrics
sequenceDiagram
participant Serializer as Serializer
participant Meta as AnnotationMeta
participant Spelling as SpellingUtil
participant Output as OutputStream
Serializer->>Meta: query enum_string & enum_policy
Meta-->>Serializer: enum_string=true, policy
Serializer->>Spelling: map_enum_to_string(value, policy)
Spelling-->>Serializer: string
Serializer->>Output: write field key/value
Serializer->>Meta: check map key parseable type
Meta-->>Serializer: parseable_map_key<Key>
Serializer->>Spelling: map_key_to_string(key)
Spelling-->>Serializer: stringified key
Serializer->>Output: write key-value pair
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Remaining comments which cannot be posted as a review comment to avoid GitHub Rate Limit
fmt
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 672 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 756 to 758 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 1023 to 1024 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 1027 to 1028 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 1683 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 1690 to 1691 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 1791 to 1793 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 1910 to 1911 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 2242 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 2429 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 2461 to 2462 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 2465 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 2468 to 2469 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 2472 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 2774 to 2776 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 2877 to 2879 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 2904 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 2989 to 2990 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 2993 to 2994 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 2997 to 2998 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 3001 to 3002 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 3005 to 3006 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 3117 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 3397 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 3453 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 3593 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 3758 to 3761 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 3865 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 3868 to 3869 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 3930 to 3931 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 3941 to 3943 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 3955 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 3973 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 4011 to 4012 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 4016 to 4017 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 4049 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 4059 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 4067 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 4167 to 4168 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 4179 to 4181 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 4184 to 4185 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 4216 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 4721 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 4728 to 4730 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 4788 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 4801 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 4822 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 4953 to 4954 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 4962 to 4963 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 4976 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 4982 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 4996 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 5125 to 5127 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 5130 to 5131 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 5687 to 5690 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 5695 to 5697 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 5699 to 5700 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 5702 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 5719 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 5729 to 5734 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 5737 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 5778 to 5779 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 5807 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 5921 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 5934 to 5935 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 5938 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 5947 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 5950 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Lines 5953 to 5955 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6022 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6077 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6104 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6129 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6143 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6153 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6159 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6195 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6215 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6261 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6278 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6557 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6569 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6575 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6578 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6601 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6610 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6621 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6624 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6632 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6637 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6642 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6652 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6657 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/include/language/protocol.h
Line 6662 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/scripts/lsp_codegen.py
Line 1196 in 7f07059
[fmt] reported by reviewdog 🐶
eventide/scripts/lsp_codegen.py
Lines 1242 to 1244 in 7f07059
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
include/serde/spelling.h (2)
240-286: Robust enum-string round-trip logic with good fallback strategies.The multi-strategy parsing in
map_string_to_enum(keyword-suffixed names, digit-prefixed names, camelCase retries) handles the naming quirks of LSP code generation well.One minor observation: the function is declared
constexprbut calls non-constexpr helpers (normalize_to_lower_snake,snake_to_camel), so it won't be evaluable at compile time. Not a bug — the compiler will just treat it as a regular function — but theconstexprspecifier is misleading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/serde/spelling.h` around lines 240 - 286, The function map_string_to_enum is marked constexpr but calls non-constexpr helpers (detail::snake_to_camel and other normalization helpers), so change its declaration to remove the constexpr specifier; update the signature of map_string_to_enum<...> to be a normal function (not constexpr) so the declaration matches its non-constexpr behavior and avoids misleading callers, referencing the function name map_string_to_enum and the helper calls detail::snake_to_camel and any normalize_to_lower_snake usages when making the edit.
113-136: Minor: digits after underscores don't trigger capitalization of subsequent alpha characters.In
snake_to_camel, when a digit follows an underscore,capitalize_nextis consumed by the digit (which is not alpha), so the subsequent alpha character won't be capitalized. For example,"foo_2bar"→"foo2bar"rather than"foo2Bar". If this is intentional for LSP enum naming, it's fine; otherwise the digit should not resetcapitalize_next.Potential fix if the behavior is unintended
if(c == '_') { capitalize_next = true; continue; } if(capitalize_next && is_ascii_alpha(c)) { out.push_back(ascii_upper(c)); - } else if(!seen_output) { + capitalize_next = false; + } else if(capitalize_next && is_ascii_digit(c)) { + out.push_back(c); + // keep capitalize_next = true for next alpha + } else if(!seen_output) { out.push_back(upper_first ? ascii_upper(c) : ascii_lower(c)); + capitalize_next = false; } else { out.push_back(c); + capitalize_next = false; } - capitalize_next = false; seen_output = true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/serde/spelling.h` around lines 113 - 136, The snake_to_camel function currently consumes capitalize_next for any non-underscore character (including digits), so "foo_2bar" becomes "foo2bar" instead of "foo2Bar"; update snake_to_camel (and its local flags capitalize_next and seen_output) so that capitalize_next is only cleared when an alphabetic character is processed. Specifically, leave the underscore handling as-is, but when iterating characters ensure digits (and other non-alpha non-underscore chars) are appended without resetting capitalize_next, and only set capitalize_next = false inside the branches that handle alphabetic conversion (where you call is_ascii_alpha and ascii_upper/ascii_lower), preserving seen_output behavior; refer to symbols snake_to_camel, normalize_to_lower_snake, capitalize_next, seen_output, is_ascii_alpha, ascii_upper, and ascii_lower.include/serde/serde.h (1)
369-388: Silent fallback to default enum value on unknown strings.When
map_string_to_enumreturnsstd::nullopt(unrecognized string), the code silently assignsenum_t{}(zero-initialized). This is likely intentional for LSP forward-compatibility (unknown enum values from newer protocol versions), but it could mask protocol errors or data corruption.Consider whether logging/tracing the fallback or providing an opt-in strict mode would be valuable for debugging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@include/serde/serde.h` around lines 369 - 388, The code currently falls back silently to enum_t{} when serde::detail::map_string_to_enum<enum_t, typename meta::enum_policy>(enum_text) returns nullopt; change this to either emit a trace/log and then fallback or to fail fast in a strict opt-in mode: when parsed.has_value() is false, call a logging/tracing helper (e.g. serde::detail::report_unknown_enum(enum_text, typeid(enum_t).name()) or similar) and keep the fallback, and additionally honor a strict flag on typename meta::enum_policy (e.g. meta::enum_policy::strict) so that when strict is true the code returns std::unexpected(...) instead of assigning enum_t{}; make these changes inside the block that uses enum_t, map_string_to_enum, deserialize_value and d_struct so behavior is controlled by meta::enum_policy.scripts/lsp_codegen.py (3)
344-344: Audit:urlopenaccepts arbitrary URL schemes.The
urlparameter comes from--fetch-urlCLI arg and is passed directly tourlopen, which acceptsfile://and other schemes. For a dev-only code generator this is low risk, but consider validating the scheme if this script could be invoked in CI with untrusted input.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lsp_codegen.py` at line 344, The call to urllib.request.urlopen(source, ...) accepts arbitrary schemes and the CLI --fetch-url value is used directly; before calling urlopen (the line using urlopen and the variable source), parse the URL with urllib.parse.urlparse and enforce allowed schemes (e.g. "http" and "https"), returning an error/exit if the scheme is missing or not allowed; update the code path that handles fetching (the code that constructs/uses source) to validate and reject unsafe schemes rather than passing them to urlopen.
888-890: Dead/misleading ternary branch; would be anIndexErrorif reached.
commentsis guaranteed non-empty by lines 885–886, soif commentsis always true. However, if that guarantee ever broke, theelse suffixpath would crash withIndexErrorbecause it still assigns tocomments[-1]on an empty list. Simplify:if inherited_from is not None: suffix = f"(Inherited from [{inherited_from}])" - comments[-1] = f"{comments[-1]} {suffix}" if comments else suffix + comments[-1] = f"{comments[-1]} {suffix}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lsp_codegen.py` around lines 888 - 890, The conditional ternary around updating comments is misleading and invalid if comments were ever empty; since lines above ensure comments is non-empty, remove the unnecessary if/else and directly append the suffix to the last comment: compute suffix from inherited_from and set comments[-1] = f"{comments[-1]} {suffix}" (use the existing inherited_from/suffix/comments variables). Alternatively, if you want defensive code, replace the ternary with a simple if comments: comments[-1] = f"{comments[-1]} {suffix}" else: comments.append(suffix).
803-806: Addstrict=Truetozip()for defensive consistency.The length check on line 801 makes this safe, but adding
strict=Truedocuments the invariant and guards against future refactors that might remove the length check.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lsp_codegen.py` around lines 803 - 806, The zip over child_items and parent_items in the generator passed to all(...) should be called with strict=True to enforce that both iterables have equal length; update the expression that yields self.is_type_subtype(child_item, parent_item) for child_item, parent_item in zip(child_items, parent_items) to use zip(child_items, parent_items, strict=True) so that the existing length invariant is documented and guarded against future refactors (this concerns the code that calls self.is_type_subtype with child_items and parent_items).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@include/language/ts.h`:
- Around line 51-73: The aliases LSPArray and LSPObject currently use LSPAny
while LSPAny is incomplete, which is undefined behavior on some toolchains;
change them to hold an indirection to a complete type (e.g., use
std::vector<std::unique_ptr<LSPAny>> and std::unordered_map<std::string,
std::unique_ptr<LSPAny>>) or alternatively std::unique_ptr<LSPVariant> if you
prefer to store the variant type; update any code constructing or accessing
these containers to allocate and dereference the unique_ptrs and keep the type
names (LSPArray, LSPObject, LSPAny, LSPVariant) so callers can be updated
consistently.
In `@include/serde/serde.h`:
- Around line 686-701: The loop handling parsed_key failure doesn't consume the
corresponding value when d_map->invalid_key(**key) returns success (lenient
mode), leaving has_pending_value true and breaking the next next_key() call;
before the continue in the invalid_key success branch inside the parsed_key
handling, call the routine that discards the pending value (invoke skip_value()
/ the function used to skip the current pending value in this parser) so the
pending value is cleared and control can safely continue; update the branch
handling d_map->invalid_key(**key) success in the code around parsed_key and
has_pending_value/next_key to call skip_value() before continuing.
In `@scripts/lsp_codegen.py`:
- Around line 1348-1349: Change the misleading print in the else branch that
currently outputs "[WARNING] no suspicious optional-bool defaults detected" to
use the "[INFO]" prefix instead, i.e. update the print statement that prints "no
suspicious optional-bool defaults detected" so it matches the informational
pattern used on the earlier line (the same message prefix as on line 1342).
- Around line 497-501: The branch handling inline literal types currently
collapses every literal (when kind == "literal") to "LspEmptyObject" even if
type_expr.get("value", {}).get("properties", []) is non-empty; change this so
non-empty inline object literals emit a meaningful type instead of silently
dropping structure: either generate an anonymous struct type from the properties
(preferred) or fall back to a more honest representation such as "json::Value"
and include a clear // TODO comment explaining the fallback; update the code
paths that currently return "LspEmptyObject" to inspect properties and return
the generated anonymous struct name or the fallback, keeping the empty-case
return as-is.
---
Nitpick comments:
In `@include/serde/serde.h`:
- Around line 369-388: The code currently falls back silently to enum_t{} when
serde::detail::map_string_to_enum<enum_t, typename meta::enum_policy>(enum_text)
returns nullopt; change this to either emit a trace/log and then fallback or to
fail fast in a strict opt-in mode: when parsed.has_value() is false, call a
logging/tracing helper (e.g. serde::detail::report_unknown_enum(enum_text,
typeid(enum_t).name()) or similar) and keep the fallback, and additionally honor
a strict flag on typename meta::enum_policy (e.g. meta::enum_policy::strict) so
that when strict is true the code returns std::unexpected(...) instead of
assigning enum_t{}; make these changes inside the block that uses enum_t,
map_string_to_enum, deserialize_value and d_struct so behavior is controlled by
meta::enum_policy.
In `@include/serde/spelling.h`:
- Around line 240-286: The function map_string_to_enum is marked constexpr but
calls non-constexpr helpers (detail::snake_to_camel and other normalization
helpers), so change its declaration to remove the constexpr specifier; update
the signature of map_string_to_enum<...> to be a normal function (not constexpr)
so the declaration matches its non-constexpr behavior and avoids misleading
callers, referencing the function name map_string_to_enum and the helper calls
detail::snake_to_camel and any normalize_to_lower_snake usages when making the
edit.
- Around line 113-136: The snake_to_camel function currently consumes
capitalize_next for any non-underscore character (including digits), so
"foo_2bar" becomes "foo2bar" instead of "foo2Bar"; update snake_to_camel (and
its local flags capitalize_next and seen_output) so that capitalize_next is only
cleared when an alphabetic character is processed. Specifically, leave the
underscore handling as-is, but when iterating characters ensure digits (and
other non-alpha non-underscore chars) are appended without resetting
capitalize_next, and only set capitalize_next = false inside the branches that
handle alphabetic conversion (where you call is_ascii_alpha and
ascii_upper/ascii_lower), preserving seen_output behavior; refer to symbols
snake_to_camel, normalize_to_lower_snake, capitalize_next, seen_output,
is_ascii_alpha, ascii_upper, and ascii_lower.
In `@scripts/lsp_codegen.py`:
- Line 344: The call to urllib.request.urlopen(source, ...) accepts arbitrary
schemes and the CLI --fetch-url value is used directly; before calling urlopen
(the line using urlopen and the variable source), parse the URL with
urllib.parse.urlparse and enforce allowed schemes (e.g. "http" and "https"),
returning an error/exit if the scheme is missing or not allowed; update the code
path that handles fetching (the code that constructs/uses source) to validate
and reject unsafe schemes rather than passing them to urlopen.
- Around line 888-890: The conditional ternary around updating comments is
misleading and invalid if comments were ever empty; since lines above ensure
comments is non-empty, remove the unnecessary if/else and directly append the
suffix to the last comment: compute suffix from inherited_from and set
comments[-1] = f"{comments[-1]} {suffix}" (use the existing
inherited_from/suffix/comments variables). Alternatively, if you want defensive
code, replace the ternary with a simple if comments: comments[-1] =
f"{comments[-1]} {suffix}" else: comments.append(suffix).
- Around line 803-806: The zip over child_items and parent_items in the
generator passed to all(...) should be called with strict=True to enforce that
both iterables have equal length; update the expression that yields
self.is_type_subtype(child_item, parent_item) for child_item, parent_item in
zip(child_items, parent_items) to use zip(child_items, parent_items,
strict=True) so that the existing length invariant is documented and guarded
against future refactors (this concerns the code that calls self.is_type_subtype
with child_items and parent_items).
| else: | ||
| print("[WARNING] no suspicious optional-bool defaults detected") |
There was a problem hiding this comment.
Misleading [WARNING] prefix for an informational "all clear" message.
When there are no suspicious defaults, printing [WARNING] no suspicious optional-bool defaults detected is confusing — it looks like a warning when it's actually an all-clear status. Use [INFO] instead, matching the pattern on line 1342.
else:
- print("[WARNING] no suspicious optional-bool defaults detected")
+ print("[INFO] no suspicious optional-bool defaults detected")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/lsp_codegen.py` around lines 1348 - 1349, Change the misleading print
in the else branch that currently outputs "[WARNING] no suspicious optional-bool
defaults detected" to use the "[INFO]" prefix instead, i.e. update the print
statement that prints "no suspicious optional-bool defaults detected" so it
matches the informational pattern used on the earlier line (the same message
prefix as on line 1342).
There was a problem hiding this comment.
🧹 Nitpick comments (6)
scripts/lsp_codegen.py (6)
803-806:zip()withoutstrict=True.The length check on line 801 makes this safe at runtime, but adding
strict=Truewould make the intent self-documenting and guard against future refactors that might remove the length check.return all( self.is_type_subtype(child_item, parent_item) - for child_item, parent_item in zip(child_items, parent_items) + for child_item, parent_item in zip(child_items, parent_items, strict=True) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lsp_codegen.py` around lines 803 - 806, Update the zip call in the is_type_subtype logic to use strict=True so the pairwise iteration enforces equal lengths even if the earlier length check is removed in the future; specifically change the generator in the return statement that iterates over child_items and parent_items (the zip(...) inside the is_type_subtype-related code) to pass strict=True. Ensure the environment supports Python 3.10+ where zip(strict=True) is available.
888-890: Deadelsebranch in ternary —commentsis always non-empty here.Lines 885–886 guarantee
commentshas at least one element, so theif comments else suffixternary's false branch is unreachable. Simplify for clarity:if inherited_from is not None: suffix = f"(Inherited from [{inherited_from}])" - comments[-1] = f"{comments[-1]} {suffix}" if comments else suffix + comments[-1] = f"{comments[-1]} {suffix}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lsp_codegen.py` around lines 888 - 890, The ternary assigning to comments[-1] is dead because comments is guaranteed non-empty when inherited_from is not None; replace the ternary expression in the block handling inherited_from with a direct append to the last comment: compute suffix = f"(Inherited from [{inherited_from}])" and set comments[-1] = f"{comments[-1]} {suffix}" (remove the "if comments else suffix" branch) to simplify the code around the inherited_from handling.
1228-1237: Return typedict[str, object]forces# type: ignoreat every call site.The
generate_filesreturn value has a known shape but is typed asdict[str, object], requiring# type: ignore[assignment]on lines 1337, 1344, 1351, and 1355. ATypedDictor a@dataclasswould eliminate those suppressions and make the contract explicit.Sketch using a dataclass
`@dataclass` class GenerationSummary: struct_count: int enum_count: int alias_count: int output_file: str keyword_hits: list[str] bool_warnings: list[str] unsafe_overrides: list[str] member_collisions: list[str]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lsp_codegen.py` around lines 1228 - 1237, The current generate_files function returns an untyped dict (with keys like "struct_count", "enum_count", "alias_count", "output_file", and lists from generator.keyword_hits, generator.bool_default_warnings, generator.unsafe_override_warnings, generator.member_collision_warnings) which forces callers to use # type: ignore; define a concrete return type (either a TypedDict or a `@dataclass` named e.g. GenerationSummary) with typed fields matching those keys (struct_count:int, enum_count:int, alias_count:int, output_file:str, keyword_hits:list[str], bool_warnings:list[str], unsafe_overrides:list[str], member_collisions:list[str]), update generate_files to return that type instead of dict[str, object], adjust import typing/dataclasses, and remove the # type: ignore suppressions at the call sites (ensure callers expect GenerationSummary and access the same attributes).
1209-1210: Duplicate ofbuild_node_order()logic.
generator.build_node_order()(line 713) already wrapsbuild_node_dependencies()+topological_order(). Consider using it directly:- nodes, node_deps = generator.build_node_dependencies() - node_order = topological_order(nodes, node_deps) + node_order = generator.build_node_order()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lsp_codegen.py` around lines 1209 - 1210, Replace the manual two-step sequence that calls generator.build_node_dependencies() and topological_order() with the single helper generator.build_node_order() call: remove the nodes, node_deps = generator.build_node_dependencies() and node_order = topological_order(nodes, node_deps) lines and instead call generator.build_node_order() to obtain the node_order (assign its return to the same node_order variable and update any downstream usage if the helper returns a different shape). This eliminates the duplicate logic and uses the existing build_node_order() wrapper.
344-344: URL scheme is not validated before opening.
urllib.request.urlopenacceptsfile://and other non-HTTP schemes. Since the URL can be user-supplied via--fetch-url, consider restricting tohttps://(orhttp://) to avoid unintended local file reads, even though this is a developer-facing CLI tool.Proposed fix
+ parsed_url = urllib.parse.urlparse(source) + if parsed_url.scheme not in ("http", "https"): + raise RuntimeError(f"unsupported URL scheme: {parsed_url.scheme!r}") + try: with urllib.request.urlopen(source, timeout=timeout) as response:This would also require adding
import urllib.parseat the top of the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lsp_codegen.py` at line 344, The call to urllib.request.urlopen(source, timeout=timeout) is unguarded and can open non-HTTP schemes (e.g., file://); update the fetch routine that reads the user-supplied --fetch-url (the code using the variable source and calling urllib.request.urlopen) to validate the URL scheme first using urllib.parse.urlparse and only allow http or https schemes (explicitly reject or raise an error for other schemes), and add import urllib.parse at the top of the file; ensure the error message clearly indicates the invalid scheme and that only http/https are permitted.
395-400: Cycles in the dependency graph are silently absorbed.When
len(ordered) != len(nodes), the remaining (cyclically-dependent) nodes are appended without any diagnostic. This can produce a generated header with forward-reference errors that are hard to trace back to a dependency cycle. Consider emitting a warning listing the affected nodes.Proposed fix
if len(ordered) != len(nodes): existing = set(ordered) + cycle_nodes = sorted(n for n in nodes if n not in existing) + import sys as _sys + print( + f"[WARNING] dependency cycle detected; appending {len(cycle_nodes)} node(s) in arbitrary order: " + + ", ".join(f"{k}:{n}" for k, n in cycle_nodes), + file=_sys.stderr, + ) for node in sorted(nodes): if node not in existing: ordered.append(node)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/lsp_codegen.py` around lines 395 - 400, The topological-sort logic silently appends remaining cyclic nodes (when len(ordered) != len(nodes)); change it to detect the cycle, compute the cyclic set as remaining = sorted(set(nodes) - set(ordered)), log or warn (e.g., using logging.warning or sys.stderr) with a clear message that names the affected nodes before proceeding, then (if desired) append them as the fallback; update the block that currently builds existing/ordered to emit this warning and include the list of remaining node names for diagnostics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@scripts/lsp_codegen.py`:
- Around line 497-501: The literal-type branch currently collapses both empty
and non-empty inline object literals to "LspEmptyObject"; update the branch
under kind == "literal" (the code that reads properties via
type_expr.get("value", {}).get("properties", [])) so that only the
empty-properties case returns "LspEmptyObject" and non-empty inline object
literals are preserved (either add a TODO comment explaining this limitation on
the non-empty branch or return a safe fallback such as "serde_json::Value" /
"json::Value" instead of "LspEmptyObject").
- Around line 1348-1349: Replace the misleading print in the else branch that
emits "[WARNING] no suspicious optional-bool defaults detected" with an
informational message; update the string to "[INFO] no suspicious optional-bool
defaults detected" where the print call is in scripts/lsp_codegen.py (the else
branch that prints the all-clear message) so it matches the "[INFO]" pattern
used earlier around line 1342.
---
Nitpick comments:
In `@scripts/lsp_codegen.py`:
- Around line 803-806: Update the zip call in the is_type_subtype logic to use
strict=True so the pairwise iteration enforces equal lengths even if the earlier
length check is removed in the future; specifically change the generator in the
return statement that iterates over child_items and parent_items (the zip(...)
inside the is_type_subtype-related code) to pass strict=True. Ensure the
environment supports Python 3.10+ where zip(strict=True) is available.
- Around line 888-890: The ternary assigning to comments[-1] is dead because
comments is guaranteed non-empty when inherited_from is not None; replace the
ternary expression in the block handling inherited_from with a direct append to
the last comment: compute suffix = f"(Inherited from [{inherited_from}])" and
set comments[-1] = f"{comments[-1]} {suffix}" (remove the "if comments else
suffix" branch) to simplify the code around the inherited_from handling.
- Around line 1228-1237: The current generate_files function returns an untyped
dict (with keys like "struct_count", "enum_count", "alias_count", "output_file",
and lists from generator.keyword_hits, generator.bool_default_warnings,
generator.unsafe_override_warnings, generator.member_collision_warnings) which
forces callers to use # type: ignore; define a concrete return type (either a
TypedDict or a `@dataclass` named e.g. GenerationSummary) with typed fields
matching those keys (struct_count:int, enum_count:int, alias_count:int,
output_file:str, keyword_hits:list[str], bool_warnings:list[str],
unsafe_overrides:list[str], member_collisions:list[str]), update generate_files
to return that type instead of dict[str, object], adjust import
typing/dataclasses, and remove the # type: ignore suppressions at the call sites
(ensure callers expect GenerationSummary and access the same attributes).
- Around line 1209-1210: Replace the manual two-step sequence that calls
generator.build_node_dependencies() and topological_order() with the single
helper generator.build_node_order() call: remove the nodes, node_deps =
generator.build_node_dependencies() and node_order = topological_order(nodes,
node_deps) lines and instead call generator.build_node_order() to obtain the
node_order (assign its return to the same node_order variable and update any
downstream usage if the helper returns a different shape). This eliminates the
duplicate logic and uses the existing build_node_order() wrapper.
- Line 344: The call to urllib.request.urlopen(source, timeout=timeout) is
unguarded and can open non-HTTP schemes (e.g., file://); update the fetch
routine that reads the user-supplied --fetch-url (the code using the variable
source and calling urllib.request.urlopen) to validate the URL scheme first
using urllib.parse.urlparse and only allow http or https schemes (explicitly
reject or raise an error for other schemes), and add import urllib.parse at the
top of the file; ensure the error message clearly indicates the invalid scheme
and that only http/https are permitted.
- Around line 395-400: The topological-sort logic silently appends remaining
cyclic nodes (when len(ordered) != len(nodes)); change it to detect the cycle,
compute the cyclic set as remaining = sorted(set(nodes) - set(ordered)), log or
warn (e.g., using logging.warning or sys.stderr) with a clear message that names
the affected nodes before proceeding, then (if desired) append them as the
fallback; update the block that currently builds existing/ordered to emit this
warning and include the list of remaining node names for diagnostics.
Summary by CodeRabbit
New Features
Chores