From eee633b9bec8391ab79e75b2fb942b9e2bdbd747 Mon Sep 17 00:00:00 2001 From: Oleg-Melnik Date: Tue, 7 Jul 2026 17:18:55 +0300 Subject: [PATCH 1/3] Add tests that reproduces the panic line number issue. --- embedding/embedding_test.go | 30 ++++++-- embedding/parsing/state_test.go | 120 ++++++++++++++++++++++++++++++++ 2 files changed, 146 insertions(+), 4 deletions(-) diff --git a/embedding/embedding_test.go b/embedding/embedding_test.go index b0ddb12..3de2ad1 100644 --- a/embedding/embedding_test.go +++ b/embedding/embedding_test.go @@ -36,6 +36,7 @@ import ( "embed-code/embed-code-go/configuration" "embed-code/embed-code-go/embedding" "embed-code/embed-code-go/embedding/parsing" + "embed-code/embed-code-go/logging" _type "embed-code/embed-code-go/type" . "github.com/onsi/ginkgo/v2" @@ -142,6 +143,27 @@ var _ = Describe("Embedding", func() { Expect(errors.As(err, &parseErr)).Should(BeTrue()) }) + // Regresses https://github.com/SpineEventEngine/embed-code-go/issues/19. + // + // The failure location must be a bare `file://...:line:column` URL so an + // IDE such as IntelliJ IDEA can open it from the console. Backticks around + // the URL and a missing column component prevent that navigation. + It("should format the failure location as a bare file URL with line and column", func() { + docPath := filepath.Join(GinkgoT().TempDir(), "doc.md") + processingErr := embedding.ProcessingError{ + DocFilePath: docPath, + Line: 2, + Err: errors.New("boom"), + } + + message := processingErr.Error() + + Expect(message).Should(ContainSubstring( + logging.FileReferenceWithPosition(docPath, 2, 1))) + Expect(message).Should(ContainSubstring(":2:1")) + Expect(message).ShouldNot(ContainSubstring("`")) + }) + It("should report all pattern matching errors", func() { config.DocIncludes = []string{"missing-start-pattern.md", "missing-end-pattern.md"} @@ -243,7 +265,7 @@ var _ = Describe("Embedding", func() { Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring( - "missing-closing-tag.md:3`: " + + "missing-closing-tag.md:3:1: " + "failed to parse an embedding instruction: " + "the `` tag is not closed", )) @@ -275,7 +297,7 @@ var _ = Describe("Embedding", func() { Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring( - "unclosed-nested-tag.md:3`: " + + "unclosed-nested-tag.md:3:1: " + "failed to parse an embedding instruction: " + "element closed by ", )) @@ -289,7 +311,7 @@ var _ = Describe("Embedding", func() { Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring( - "missing-code-fence.md:3`: " + + "missing-code-fence.md:3:1: " + "expected a markdown code fence after the embedding instruction", )) }) @@ -302,7 +324,7 @@ var _ = Describe("Embedding", func() { Expect(err).Should(HaveOccurred()) Expect(err.Error()).Should(ContainSubstring( - "unclosed-code-fence.md:3`: " + + "unclosed-code-fence.md:3:1: " + "the markdown code fence after the embedding instruction is not closed", )) }) diff --git a/embedding/parsing/state_test.go b/embedding/parsing/state_test.go index c2452b0..cc766e8 100644 --- a/embedding/parsing/state_test.go +++ b/embedding/parsing/state_test.go @@ -120,6 +120,100 @@ var _ = Describe("Parser states", func() { })) }) + // The following specs reproduce + // https://github.com/SpineEventEngine/embed-code-go/issues/19. + // + // A self-closed `` instruction that fails validation is a + // complete, single-line instruction, yet `Accept` keeps re-parsing the + // joined body and consumes every following line up to EOF before reporting + // the failure. That is why a single malformed instruction near the top of a + // document surfaces its error many lines later (line 76 -> line 404 in the + // original report) and destroys the parse of everything after it. + // + // The parser should report the InstructionParseError but stop right after + // the instruction's `/>` terminator, leaving the code fence and the rest of + // the document intact. These specs assert that bounded behavior and + // therefore FAIL until the over-consumption is fixed. + It("should not consume the rest of the document when a start pattern is invalid", func() { + assertBoundedMalformedInstruction(``) + }) + + It("should not consume the rest of the document when the comments mode is invalid", func() { + assertBoundedMalformedInstruction(``) + }) + + It("should not consume the rest of the document when attributes are mutually exclusive", func() { + assertBoundedMalformedInstruction(``) + }) + + // Reproduces https://github.com/SpineEventEngine/embed-code-go/issues/19. + // + // A self-closed instruction whose start/end/line pattern contains raw XML + // metacharacters (`<`, `&`) is legitimate user input (e.g. matching a Java + // generic or comparison), yet `quoteEscapedXMLLine` only escapes `\"`, so + // `xml.Unmarshal` rejects it. Because the tag is self-closed, this is NOT + // the "tag is not closed" case; instead `Accept` keeps re-parsing and + // consumes every following line up to EOF, then fails with + // InstructionParseError -- swallowing the code fence and the rest of the + // document, exactly the behavior reported in the issue. + // + // This test asserts the desired behavior and therefore FAILS until the XML + // metacharacters in attribute values are escaped before unmarshalling. + It("should parse an instruction whose pattern contains XML metacharacters", func() { + config := configuration.NewConfiguration() + context := newStateContext( + "", + "```java", + "old source", + "```", + "text after the fence", + ) + + Expect(parsing.EmbedInstruction.Recognize(context)).Should(BeTrue()) + + err := parsing.EmbedInstruction.Accept(&context, config) + + Expect(err).ShouldNot(HaveOccurred()) + Expect(context.EmbeddingInstruction).ShouldNot(BeNil()) + Expect(context.EmbeddingInstruction.LinePattern).ShouldNot(BeNil()) + // The parser must stop right after the instruction line rather than + // swallowing the code fence and the rest of the document. + Expect(context.ReachedEOF()).Should(BeFalse()) + Expect(context.GetResult()).Should(Equal([]string{ + "", + })) + }) + + // Regresses https://github.com/SpineEventEngine/embed-code-go/issues/19. + // + // The `/>` inside the `line` value must not be mistaken for the tag + // terminator: the instruction spans several lines and only closes on the + // last one. Recognizing the inner `/>` would stop accumulation early and + // fail with an "unexpected EOF" before the real closing `/>`. + It("should accept a multiline instruction whose value contains a slash-close sequence", func() { + config := configuration.NewConfiguration() + context := newStateContext( + "`, + "```java", + "old source", + "```", + ) + + Expect(parsing.EmbedInstruction.Recognize(context)).Should(BeTrue()) + Expect(parsing.EmbedInstruction.Accept(&context, config)).Should(Succeed()) + + Expect(context.EmbeddingInstruction).ShouldNot(BeNil()) + Expect(context.EmbeddingInstruction.CodeFile).Should(Equal("Example.java")) + Expect(context.EmbeddingInstruction.LinePattern).ShouldNot(BeNil()) + Expect(context.GetResult()).Should(Equal([]string{ + "`, + })) + }) + It("should render source and close the embedding fence when the end state is accepted", func() { sourceRoot := GinkgoT().TempDir() Expect(os.WriteFile( @@ -181,6 +275,32 @@ var _ = Describe("Parser states", func() { }) }) +// assertBoundedMalformedInstruction drives EmbedInstruction.Accept over a +// document whose first line is a self-closed but invalid instruction, followed +// by a code fence and trailing content. +// +// It asserts that the parser reports the failure as an InstructionParseError +// without consuming past the instruction line. See issue #19. +func assertBoundedMalformedInstruction(instruction string) { + config := configuration.NewConfiguration() + context := newStateContext( + instruction, + "```java", + "old source", + "```", + "text after the fence", + ) + + Expect(parsing.EmbedInstruction.Recognize(context)).Should(BeTrue()) + + err := parsing.EmbedInstruction.Accept(&context, config) + + var parseErr parsing.InstructionParseError + Expect(errors.As(err, &parseErr)).Should(BeTrue()) + Expect(context.ReachedEOF()).Should(BeFalse()) + Expect(context.GetResult()).Should(Equal([]string{instruction})) +} + // newStateContext builds a parser context from in-memory source lines. func newStateContext(lines ...string) parsing.Context { docPath := filepath.Join(GinkgoT().TempDir(), "doc.md") From ba56d9ded0c8d70467a1d730c5aaf49d0d313e2d Mon Sep 17 00:00:00 2001 From: Oleg-Melnik Date: Tue, 7 Jul 2026 17:19:19 +0300 Subject: [PATCH 2/3] Implement fixes for the panic line number issue. --- embedding/error.go | 13 ++- embedding/parsing/instruction_token.go | 44 +++++++++- embedding/parsing/xml_parse.go | 116 ++++++++++++++++++++++++- logging/logger.go | 25 ++++++ 4 files changed, 191 insertions(+), 7 deletions(-) diff --git a/embedding/error.go b/embedding/error.go index 4842d79..32f9402 100644 --- a/embedding/error.go +++ b/embedding/error.go @@ -44,13 +44,22 @@ type ProcessingError struct { Err error } +// errorLocationColumn is the column reported for a processing failure. The +// parser tracks line granularity only, so the location points at the beginning +// of the offending line. +const errorLocationColumn = 1 + // Error returns a user-facing description of the failed documentation processing operation. // +// The documentation location is emitted as a bare `file://...:line:column` +// URL, without surrounding backticks, so IDEs such as IntelliJ IDEA can open +// it from the console. +// // Returns formatted processing error text. func (e ProcessingError) Error() string { return fmt.Sprintf( - "failed to embed code fragment into doc file `%s`: %s", - logging.FileReferenceWithLine(e.DocFilePath, e.Line), + "failed to embed code fragment into doc file %s: %s", + logging.FileReferenceWithPosition(e.DocFilePath, e.Line, errorLocationColumn), e.Err, ) } diff --git a/embedding/parsing/instruction_token.go b/embedding/parsing/instruction_token.go index ab4c3b6..da9d832 100644 --- a/embedding/parsing/instruction_token.go +++ b/embedding/parsing/instruction_token.go @@ -129,6 +129,13 @@ func (e EmbedInstructionTokenState) Accept(context *Context, context.Result = append(context.Result, line) context.ToNextLine() + + // Once the tag is syntactically closed, the following lines are not + // part of the instruction. Stop here instead of consuming the rest of + // the document trying to parse a complete but invalid instruction. + if context.EmbeddingInstruction == nil && instructionClosed(instructionBody) { + break + } } if context.EmbeddingInstruction == nil { return InstructionParseError{ @@ -140,11 +147,42 @@ func (e EmbedInstructionTokenState) Accept(context *Context, return nil } +// instructionClosed reports whether the accumulated instruction body contains a +// closing tag, meaning any following lines are not part of the instruction. +// +// The terminator is only recognized outside quoted attribute values, so a +// value such as `line="
"` does not end the instruction early. +func instructionClosed(instructionBody []string) bool { + instruction := strings.Join(instructionBody, " ") + closingTag := "" + insideValue := false + for i := 0; i < len(instruction); i++ { + char := instruction[i] + if char == '\\' && i+1 < len(instruction) && instruction[i+1] == '"' { + i++ + + continue + } + if char == '"' { + insideValue = !insideValue + + continue + } + if insideValue { + continue + } + if strings.HasPrefix(instruction[i:], "/>") || + strings.HasPrefix(instruction[i:], closingTag) { + return true + } + } + + return false +} + // parseFailureReason explains why an embedding instruction could not be parsed. func parseFailureReason(instructionBody []string, parseErr error) string { - instruction := strings.TrimSpace(strings.Join(instructionBody, " ")) - if !strings.Contains(instruction, "/>") && - !strings.Contains(instruction, "") { + if !instructionClosed(instructionBody) { return fmt.Sprintf("the `<%s>` tag is not closed", EmbeddingTag, ) diff --git a/embedding/parsing/xml_parse.go b/embedding/parsing/xml_parse.go index d7ab6c8..747c406 100644 --- a/embedding/parsing/xml_parse.go +++ b/embedding/parsing/xml_parse.go @@ -101,7 +101,119 @@ func ParseXMLLine(xmlLine string) (map[string]string, error) { return attributes, nil } -// quoteEscapedXMLLine converts backslash-escaped quotes into XML entities. +// quoteEscapedXMLLine escapes characters that XML forbids inside attribute +// values, so instruction patterns may contain raw source-code characters. +// +// Backslash-escaped quotes become the `"` entity, and the XML +// metacharacters `&`, `<`, and `>` appearing inside a quoted attribute value +// are escaped to their entities. Source-line patterns such as +// `line="if (a < b)"` or `end="while (x && y)"` routinely contain these +// characters; without escaping, xml.Unmarshal rejects the whole instruction +// and the parser fails with an InstructionParseError. Markup outside quoted +// values, such as the `` terminator, is left +// untouched. func quoteEscapedXMLLine(xmlLine string) string { - return strings.ReplaceAll(xmlLine, `\"`, """) + var builder strings.Builder + builder.Grow(len(xmlLine)) + insideValue := false + for i := 0; i < len(xmlLine); i++ { + char := xmlLine[i] + if char == '\\' && i+1 < len(xmlLine) && xmlLine[i+1] == '"' { + builder.WriteString(""") + i++ + + continue + } + if char == '"' { + insideValue = !insideValue + builder.WriteByte(char) + + continue + } + if insideValue { + switch char { + case '&': + // Preserve a pre-escaped entity such as `"`; escape a + // raw ampersand so xml.Unmarshal accepts it. + if entity := xmlEntityPrefix(xmlLine[i:]); entity != "" { + builder.WriteString(entity) + i += len(entity) - 1 + } else { + builder.WriteString("&") + } + + continue + case '<': + builder.WriteString("<") + + continue + case '>': + builder.WriteString(">") + + continue + } + } + builder.WriteByte(char) + } + + return builder.String() +} + +// xmlEntityPrefix returns the leading XML character entity reference in text, +// or an empty string when text does not begin with one. +// +// It recognizes the predefined entities (`&`, `<`, `>`, `"`, +// `'`) and numeric character references such as `'` or ``, so a +// pattern author may include a pre-escaped entity without it being re-escaped. +func xmlEntityPrefix(text string) string { + semicolon := strings.IndexByte(text, ';') + if semicolon < 2 { + return "" + } + name := text[1:semicolon] + switch name { + case "amp", "lt", "gt", "quot", "apos": + return text[:semicolon+1] + } + if isNumericCharRef(name) { + return text[:semicolon+1] + } + + return "" +} + +// isNumericCharRef reports whether name is the body of an XML numeric character +// reference, such as `#39` (decimal) or `#x1F` (hexadecimal). +func isNumericCharRef(name string) bool { + if len(name) < 2 || name[0] != '#' { + return false + } + digits := name[1:] + hexadecimal := digits[0] == 'x' || digits[0] == 'X' + if hexadecimal { + digits = digits[1:] + } + if digits == "" { + return false + } + for i := 0; i < len(digits); i++ { + if !isReferenceDigit(digits[i], hexadecimal) { + return false + } + } + + return true +} + +// isReferenceDigit reports whether char is a valid digit for a numeric +// character reference, allowing hexadecimal digits when hexadecimal is set. +func isReferenceDigit(char byte, hexadecimal bool) bool { + if char >= '0' && char <= '9' { + return true + } + if !hexadecimal { + return false + } + + return (char >= 'a' && char <= 'f') || (char >= 'A' && char <= 'F') } diff --git a/logging/logger.go b/logging/logger.go index dfb6929..4965638 100644 --- a/logging/logger.go +++ b/logging/logger.go @@ -159,6 +159,31 @@ func FileReferenceWithLine(path string, line int) string { return reference + ":" + strconv.Itoa(line) } +// FileReferenceWithPosition returns a clickable file URL with line and column +// suffixes. +// +// The `file://...:line:column` form is the one IDEs such as IntelliJ IDEA +// recognize for navigation; a line-only suffix is not enough. When no column +// is known, pass 1 to point at the beginning of the line. +// +// Parameters: +// path - provides a local file path. +// line - provides an optional one-based line number. +// column - provides an optional one-based column number. +// +// Returns file reference with line and column suffixes when both are positive. +func FileReferenceWithPosition(path string, line int, column int) string { + if column <= 0 { + return FileReferenceWithLine(path, line) + } + reference := FileReferenceWithLine(path, line) + if line <= 0 { + return reference + } + + return reference + ":" + strconv.Itoa(column) +} + // fileURLFromAbsolutePath formats an absolute local path as an OS-neutral file URL. func fileURLFromAbsolutePath(path string) string { normalizedPath := filepath.ToSlash(strings.ReplaceAll(path, "\\", "/")) From dbe5d1f065f558b2c65f00791118863da582566c Mon Sep 17 00:00:00 2001 From: Oleg-Melnik Date: Tue, 7 Jul 2026 17:28:42 +0300 Subject: [PATCH 3/3] Fix golangci-lint issues in instruction parsing Refactor to satisfy the CI linter: - Reduce cyclomatic complexity of quoteEscapedXMLLine by extracting isEscapedQuote and escapeValueByte helpers (cyclop). - Replace the magic number in xmlEntityPrefix with explicit empty-name handling (mnd). - Rename short loop variable `i` to `index` in the byte scanners (varnamelen), and share isEscapedQuote with instructionClosed. Co-Authored-By: Claude Opus 4.8 --- embedding/parsing/instruction_token.go | 13 +++-- embedding/parsing/xml_parse.go | 71 ++++++++++++++++---------- 2 files changed, 50 insertions(+), 34 deletions(-) diff --git a/embedding/parsing/instruction_token.go b/embedding/parsing/instruction_token.go index da9d832..5ceff49 100644 --- a/embedding/parsing/instruction_token.go +++ b/embedding/parsing/instruction_token.go @@ -156,14 +156,13 @@ func instructionClosed(instructionBody []string) bool { instruction := strings.Join(instructionBody, " ") closingTag := "" insideValue := false - for i := 0; i < len(instruction); i++ { - char := instruction[i] - if char == '\\' && i+1 < len(instruction) && instruction[i+1] == '"' { - i++ + for index := 0; index < len(instruction); index++ { + if isEscapedQuote(instruction, index) { + index++ continue } - if char == '"' { + if instruction[index] == '"' { insideValue = !insideValue continue @@ -171,8 +170,8 @@ func instructionClosed(instructionBody []string) bool { if insideValue { continue } - if strings.HasPrefix(instruction[i:], "/>") || - strings.HasPrefix(instruction[i:], closingTag) { + if strings.HasPrefix(instruction[index:], "/>") || + strings.HasPrefix(instruction[index:], closingTag) { return true } } diff --git a/embedding/parsing/xml_parse.go b/embedding/parsing/xml_parse.go index 747c406..fd3719c 100644 --- a/embedding/parsing/xml_parse.go +++ b/embedding/parsing/xml_parse.go @@ -116,42 +116,25 @@ func quoteEscapedXMLLine(xmlLine string) string { var builder strings.Builder builder.Grow(len(xmlLine)) insideValue := false - for i := 0; i < len(xmlLine); i++ { - char := xmlLine[i] - if char == '\\' && i+1 < len(xmlLine) && xmlLine[i+1] == '"' { + for index := 0; index < len(xmlLine); index++ { + if isEscapedQuote(xmlLine, index) { builder.WriteString(""") - i++ + index++ continue } + char := xmlLine[index] if char == '"' { insideValue = !insideValue builder.WriteByte(char) continue } - if insideValue { - switch char { - case '&': - // Preserve a pre-escaped entity such as `"`; escape a - // raw ampersand so xml.Unmarshal accepts it. - if entity := xmlEntityPrefix(xmlLine[i:]); entity != "" { - builder.WriteString(entity) - i += len(entity) - 1 - } else { - builder.WriteString("&") - } - - continue - case '<': - builder.WriteString("<") - - continue - case '>': - builder.WriteString(">") - - continue - } + if escaped, consumed := escapeValueByte(xmlLine[index:], insideValue); consumed > 0 { + builder.WriteString(escaped) + index += consumed - 1 + + continue } builder.WriteByte(char) } @@ -159,6 +142,37 @@ func quoteEscapedXMLLine(xmlLine string) string { return builder.String() } +// isEscapedQuote reports whether a backslash-escaped quote (`\"`) starts at index. +func isEscapedQuote(text string, index int) bool { + return text[index] == '\\' && index+1 < len(text) && text[index+1] == '"' +} + +// escapeValueByte returns the XML escaping for the metacharacter that starts +// value together with the number of source bytes it consumes. +// +// It escapes only when insideValue is set. A pre-existing entity such as +// `"` is preserved rather than re-escaped; a raw ampersand becomes +// `&`. A consumed count of zero means the leading byte needs no escaping. +func escapeValueByte(value string, insideValue bool) (string, int) { + if !insideValue { + return "", 0 + } + switch value[0] { + case '<': + return "<", 1 + case '>': + return ">", 1 + case '&': + if entity := xmlEntityPrefix(value); entity != "" { + return entity, len(entity) + } + + return "&", 1 + default: + return "", 0 + } +} + // xmlEntityPrefix returns the leading XML character entity reference in text, // or an empty string when text does not begin with one. // @@ -167,10 +181,13 @@ func quoteEscapedXMLLine(xmlLine string) string { // pattern author may include a pre-escaped entity without it being re-escaped. func xmlEntityPrefix(text string) string { semicolon := strings.IndexByte(text, ';') - if semicolon < 2 { + if semicolon <= 0 { return "" } name := text[1:semicolon] + if name == "" { + return "" + } switch name { case "amp", "lt", "gt", "quot", "apos": return text[:semicolon+1]