Harden definitions validator against list panics and nested MDX braces - #661
Harden definitions validator against list panics and nested MDX braces#661davidnewhall wants to merge 3 commits into
Conversation
…ration cannot panic or false-flag valid JSX. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Hardens config validation and rendering against malformed list values and MDX edge cases.
Changes:
- Validates list values and definition overrides.
- Safely handles scalar list values in Compose generation.
- Improves MDX brace and code-block handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
init/config/validate.go |
Adds list validation and MDX parsing safeguards. |
init/config/definitions_test.go |
Adds regression tests for validation and MDX handling. |
init/config/compose.go |
Prevents panics from scalar list values. |
Suppressed comments (2)
init/config/validate.go:468
- This drops every line indented by four spaces, but CommonMark does not allow an indented code block to interrupt a paragraph. For example,
text\n {brokenremains paragraph/MDX content and should be rejected, yet this function removes the second line and validation succeeds. Track whether an indented code block can start (and account for container/list indentation) rather than classifying lines solely by their absolute indentation.
if leadingSpaces(line) > maxFenceIndent {
continue
init/config/validate.go:480
- CommonMark indentation is column-based and a leading tab advances to the next four-column tab stop. Returning zero for
\t{broken}means valid tab-indented code is checked as MDX; worse, `\t``` followed by invalid MDX can be mistaken for a fence opener and suppress later validation. Count leading tabs by tab stops as well as spaces.
func leadingSpaces(line string) int {
return len(line) - len(strings.TrimLeft(line, " "))
}
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| for pos := idx; pos < len(content); pos++ { | ||
| if content[pos] != '{' && content[pos] != '}' { | ||
| continue | ||
| } | ||
|
|
||
| if oddEscapes(content, pos) { | ||
| continue | ||
| } | ||
|
|
||
| if content[pos] == '{' { | ||
| depth++ | ||
| continue | ||
| } | ||
|
|
||
| depth-- | ||
| if depth == 0 { | ||
| return pos + 1 |
… JSX spans. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Suppressed comments (2)
Previously missed (2) — in code that hasn't changed since the last review.
init/config/validate.go:545
- Counting only literal spaces makes a leading tab look like zero indentation. Because
trimmedremoves the tab, a tab-indented fence is then accepted as an opener/closer even though CommonMark advances a tab to the next 4-column stop; for example, a\t```line can incorrectly close a fence and expose subsequent fenced braces to validation. Count indentation columns, including tabs.
func leadingSpaces(line string) int {
return len(line) - len(strings.TrimLeft(line, " "))
}
init/config/validate.go:533
- This treats every 4-space-indented line as code, but CommonMark indented code cannot interrupt a paragraph (and four spaces may also be normal content inside a list). For example,
paragraph\n {brokenremains paragraph/MDX content, yet this function removes the second line and validation succeeds. Please preserve block context—ideally by walking a CommonMark AST, or at least by tracking whether an indented code block can begin and list nesting—so malformed MDX in indented continuation text is still checked.
for line := range strings.SplitSeq(content, "\n") {
if leadingSpaces(line) > maxFenceIndent {
continue
![qwen-pr-bot[bot]](https://avatars.githubusercontent.com/in/4697433?s=60&v=4){kind=small}
| var out strings.Builder | ||
|
|
||
| for line := range strings.SplitSeq(content, "\n") { | ||
| if leadingSpaces(line) > maxFenceIndent { |
There was a problem hiding this comment.
Known residual (this is the indented-code point from Copilot's suppressed comment in the previous round): CommonMark says an indented code block cannot interrupt a paragraph, so text\n {broken — where the indented line is a lazy paragraph continuation and live MDX — passes validation here. If you want to close it, only treat a 4+-space line as code when the previous content line (after stripping) was blank, a fence, or another code line. Non-blocking: this is the trade this PR deliberately makes (the flipped TestMDXTildeAndLongFence expectation documents it), and the generated samples are all fence-wrapped.
| } | ||
|
|
||
| func leadingSpaces(line string) int { | ||
| return len(line) - len(strings.TrimLeft(line, " ")) |
There was a problem hiding this comment.
leadingSpaces counts spaces only. CommonMark tabs advance to the next 4-column tab stop, so a tab-indented line counts as column 0: a tab-indented code line with braces is checked as MDX (false positive), and a tab-indented backtick fence is treated as a real fence opener (can suppress later lines). The fence logic had the same space-only count before this PR, so this part is pre-existing — but the new stripIndentedCode inherits it. If you fix the paragraph-continuation case above, count a leading tab as up to 4 columns here.
There was a problem hiding this comment.
Static review only; nothing was executed. Reviewed head 48114da against base 87c0c95 (3 files, 2 commits). Public CI on the head commit is green: gotest on ubuntu/macos/windows and golangci-lint on linux/darwin/windows/freebsd all succeeded (deploy jobs skipped).
The PR bundles four distinct changes, and I read each against its callers:
- List-panic fix (
validate.go,compose.go).list/conlistdefault/example/dockerand defdefaults/examples/docker_exampleoverrides must now be sequences, andParam.Composeuses comma-ok for[]anyinstead of the forced assertion. I traced every[]anyassertion in the package: the two inComposeare now the only ones, and both are comma-ok.validateDefListOverrideschecks exactly the three mapscreateDefinedSectioncopies intoDefault/Example/Docker, so every value that can reachComposefor a list param is validated upstream, andvalidateGeneratedDocsonly renders when structural errors are empty. Fail early with a clear message, and stay safe if validation is bypassed — that layering is the right call. - Depth-balanced
{{...}}(balancedBraceEnd) with\{{x}}and\\{{x}}escaped correctly (traced both). - Fence closers now require ≤3 leading spaces, matching CommonMark's "closing code fence may be indented up to three spaces"; a 4-space-indented closer is code content.
stripIndentedCodetreats 4+-space lines as code, flipping the old false positive inTestMDXTildeAndLongFence. The flip is correct: a 4-space-indented```line is an indented code block, not MDX.
The Copilot comment (18:13) about braces inside JS strings/comments is addressed by skipJSLexical in the head commit: "}", "{", and block-comment cases now balance (verified by tracing), and an unterminated string returns -1 so broken content is still flagged.
Two non-blocking notes inlined: the paragraph-continuation gap in stripIndentedCode (the trade this PR deliberately makes), and the pre-existing space-only leadingSpaces that the new function inherits.
Nothing blocking remains. Approving; the merge decision is left to a maintainer.
… count tabs as CommonMark columns. Co-authored-by: Cursor <cursoragent@cursor.com>
Summary
Follow-up to #659 (merged accidentally). Fixes the remaining Copilot comments from the 17:16 round, the suppressed indented-code false positive, and Qwen's depth-balance note on
{{...}}.list/conlistdefaults, examples, docker values, and def overrides to be sequences before render;Composeuses comma-ok so a scalar cannot panic.{{...}}so nested objects like{{a: {b: 1}}}are accepted, and\{{x}}is not treated as a complete span.Test plan
go test ./init/config/passesgolangci-lint run ./init/config/is cleandefaulton alistparam fails validation instead of panicking{{a: {b: 1}}}is accepted;\{{x}}is still flaggedMade with Cursor