From 4e488a17bfbd9aefa187cd34fce82c05aa935a45 Mon Sep 17 00:00:00 2001 From: Conner Dunn Date: Tue, 26 May 2026 08:41:19 -0700 Subject: [PATCH 1/3] test: add failing cases for best-effort malformed slash lines ParseTask currently aborts the entire parse when a line starts with `/` but isn't a valid slash command (e.g. `//`, lone `/`, `/=foo`). These should fall through to text, preserving both the malformed line and any surrounding content. In-fence slashes are already handled by the markdown-aware path; only the bare-text case is broken. --- .../taskparser/taskparser_test.go | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) diff --git a/pkg/codingcontext/taskparser/taskparser_test.go b/pkg/codingcontext/taskparser/taskparser_test.go index 85b907df..66562d3f 100644 --- a/pkg/codingcontext/taskparser/taskparser_test.go +++ b/pkg/codingcontext/taskparser/taskparser_test.go @@ -465,6 +465,69 @@ func markdownAwareTestCases() []parseTaskCase { } } +// checkTextContains asserts the parsed task round-trips so the given substring +// is present in the reconstructed content. Used for "best-effort" cases where a +// malformed slash-command line must be preserved as text rather than aborting parse. +func checkTextContains(needles ...string) func(t *testing.T, task Task) { + return func(t *testing.T, task Task) { + t.Helper() + + got := task.String() + for _, needle := range needles { + if !strings.Contains(got, needle) { + t.Errorf("expected reconstructed task to contain %q, got %q", needle, got) + } + } + } +} + +// Malformed slash-command lines outside a code block must not abort the parse. +// The line is preserved as text. These cases currently fail with +// "failed to parse task: ... unexpected token ..." because the participle +// grammar requires Slash @Term ... and treats // (and lone /) as a hard error, +// discarding the entire task. +func bestEffortMalformedSlashCases() []parseTaskCase { + return []parseTaskCase{ + { + // Also asserts "More text" survives — guards against a fix that parses + // line-by-line and salvages only the malformed line, dropping the + // surrounding text after it. + name: "best-effort: double slash bare line outside code block", + input: "Some text\n// Just a stray double slash\nMore text\n", + check: checkTextContains("// Just a stray double slash", "More text"), + }, + { + name: "best-effort: lone slash line outside code block", + input: "Intro\n/\nMore text\n", + check: checkTextContains("Intro"), + }, + { + name: "best-effort: slash followed by non-term char outside code block", + input: "Intro\n/=oops\nMore text\n", + check: checkTextContains("Intro"), + }, + } +} + +func TestParseTask_BestEffortMalformedSlash(t *testing.T) { + t.Parallel() + + for _, tt := range bestEffortMalformedSlashCases() { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + task, err := ParseTask(tt.input) + if err != nil { + t.Fatalf("ParseTask() error = %v, want nil (malformed slash lines must not abort parse)", err) + } + + if tt.check != nil { + tt.check(t, task) + } + }) + } +} + func TestParseTask_MarkdownAware(t *testing.T) { t.Parallel() From 835d1b7a7c56bf8518ca0033ef4427434ed3f154 Mon Sep 17 00:00:00 2001 From: Conner Dunn Date: Tue, 26 May 2026 11:06:18 -0700 Subject: [PATCH 2/3] fix: best-effort fallback when slash line is invalid grammar A single malformed slash line (e.g. `//`, lone `/`, `/=foo`) outside a fenced code block caused ParseTask to abort the entire task, silently dropping the user prompt and frontmatter and corrupting downstream runs. On parser failure, parseGrammar now re-attempts line-by-line. Lines that still fail are emitted as raw text with a WARN logged at slog.Default. The whole-segment fast path is unchanged for valid input. --- .../taskparser/markdownparser.go | 43 +++++++++++++++++-- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/pkg/codingcontext/taskparser/markdownparser.go b/pkg/codingcontext/taskparser/markdownparser.go index 0cb91a1d..0521f72f 100644 --- a/pkg/codingcontext/taskparser/markdownparser.go +++ b/pkg/codingcontext/taskparser/markdownparser.go @@ -3,6 +3,7 @@ package taskparser import ( "bytes" "fmt" + "log/slog" "sort" "strings" @@ -149,19 +150,55 @@ func trailingNewlineEnd(content string, pos int) int { // parseGrammar runs the participle grammar parser on a plain text segment. // It returns nil blocks (not an error) for whitespace-only input. +// +// On parser failure (e.g. a line starting with `/` but lacking a valid term: +// `//`, lone `/`, `/=foo`), falls back to a best-effort line-by-line parse so +// one malformed line cannot discard the entire task. func parseGrammar(content string) ([]Block, error) { if strings.TrimSpace(content) == "" { return nil, nil } input, err := parser().ParseString("", content) - if err != nil { - return nil, fmt.Errorf("failed to parse task: %w", err) + if err == nil { + return input.Blocks, nil + } + + return parseGrammarBestEffort(content), nil +} + +// parseGrammarBestEffort parses content line-by-line, returning a raw text +// block for any line the grammar rejects. Called only after the whole-segment +// parse has already failed. +func parseGrammarBestEffort(content string) []Block { + var blocks []Block + + for _, line := range strings.SplitAfter(content, "\n") { + if line == "" { + continue // strings.SplitAfter emits a trailing "" when input ends with "\n" + } + + if strings.TrimSpace(line) == "" { + blocks = append(blocks, rawTextBlock(line)) + continue + } + + input, err := parser().ParseString("", line) + if err != nil { + slog.Warn("taskparser: malformed slash command, treating line as text", + "line", strings.TrimRight(line, "\r\n"), "error", err) + blocks = append(blocks, rawTextBlock(line)) + + continue + } + + blocks = append(blocks, input.Blocks...) } - return input.Blocks, nil + return blocks } + // rawTextBlock wraps a raw string as a Text block without any slash command parsing. // Content() and String() on the returned block both return the original string exactly. func rawTextBlock(content string) Block { From e8e241c9877a1c8fb53d2fda5d4f71cbe660b8f3 Mon Sep 17 00:00:00 2001 From: Conner Dunn Date: Tue, 26 May 2026 11:32:54 -0700 Subject: [PATCH 3/3] feat: inject logger through markdown and taskparser parse APIs Adds *WithLogger variants for the parse entry points so callers can route taskparser's best-effort fallback WARN logs through their own slog.Logger instead of slog.Default(): - taskparser.NewExtension(logger) (Extension stays as NewExtension(nil)) - taskparser.ParseTaskWithLogger(text, logger) - markdown.ParseMarkdownFileWithLogger(path, fm, logger) The original ParseTask/ParseMarkdownFile/Extension entry points are preserved as nil-logger wrappers, so external callers are unaffected. A nil logger resolves to slog.Default() at log time (via loggerOrDefault) rather than being captured at construction, so slog.SetDefault changes made after package load still take effect. A new test covers this. All five ParseMarkdownFile callsites and the ParseTask callsite in pkg/codingcontext/context.go now pass cc.logger. --- pkg/codingcontext/context.go | 12 ++-- pkg/codingcontext/markdown/markdown.go | 14 ++++- .../taskparser/markdownparser.go | 55 +++++++++++++------ pkg/codingcontext/taskparser/taskparser.go | 10 +++- .../taskparser/taskparser_test.go | 50 +++++++++++++++++ 5 files changed, 116 insertions(+), 25 deletions(-) diff --git a/pkg/codingcontext/context.go b/pkg/codingcontext/context.go index 47a7ba4b..cd4b198e 100644 --- a/pkg/codingcontext/context.go +++ b/pkg/codingcontext/context.go @@ -292,7 +292,7 @@ func (cc *Context) makeMarkdownWalkFunc(visitor markdownVisitor) filepath.WalkFu } var fm markdown.BaseFrontMatter - if _, parseErr := markdown.ParseMarkdownFile(path, &fm); parseErr != nil { + if _, parseErr := markdown.ParseMarkdownFileWithLogger(path, &fm, cc.logger); parseErr != nil { if cc.lintCollector != nil { var pe *markdown.ParseError if errors.As(parseErr, &pe) { @@ -386,7 +386,7 @@ func (cc *Context) findTask(taskName string) error { func (cc *Context) loadTask(path, taskName string) error { var frontMatter markdown.TaskFrontMatter - md, err := markdown.ParseMarkdownFile(path, &frontMatter) + md, err := markdown.ParseMarkdownFileWithLogger(path, &frontMatter, cc.logger) if err != nil { return fmt.Errorf("failed to parse task file %s: %w", path, err) } @@ -428,7 +428,7 @@ func (cc *Context) loadTask(path, taskName string) error { var parseErr error - task, parseErr = taskparser.ParseTask(taskContent) + task, parseErr = taskparser.ParseTaskWithLogger(taskContent, cc.logger) if parseErr != nil { return fmt.Errorf("failed to parse task content in file %s: %w", path, parseErr) } @@ -549,7 +549,7 @@ func (cc *Context) findCommand(commandName string, params taskparser.Params) (st var frontMatter markdown.CommandFrontMatter - md, err := markdown.ParseMarkdownFile(path, &frontMatter) + md, err := markdown.ParseMarkdownFileWithLogger(path, &frontMatter, cc.logger) if err != nil { return fmt.Errorf("failed to parse command file %s: %w", path, err) } @@ -816,7 +816,7 @@ func (cc *Context) findExecuteRuleFiles(ctx context.Context) error { err := cc.visitMarkdownFiles(namespacedRulePaths, func(path string, baseFm *markdown.BaseFrontMatter) error { var frontmatter markdown.RuleFrontMatter - md, err := markdown.ParseMarkdownFile(path, &frontmatter) + md, err := markdown.ParseMarkdownFileWithLogger(path, &frontmatter, cc.logger) if err != nil { return fmt.Errorf("failed to parse markdown file %s: %w", path, err) } @@ -1042,7 +1042,7 @@ func (cc *Context) loadSkillEntry(skillFile string, lenient bool) error { var frontmatter markdown.SkillFrontMatter - if _, err := markdown.ParseMarkdownFile(skillFile, &frontmatter); err != nil { + if _, err := markdown.ParseMarkdownFileWithLogger(skillFile, &frontmatter, cc.logger); err != nil { if lenient { cc.logger.Warn("skipping skill file: failed to parse YAML frontmatter", "path", skillFile, "error", err) diff --git a/pkg/codingcontext/markdown/markdown.go b/pkg/codingcontext/markdown/markdown.go index 8c1f21b2..42a60d3c 100644 --- a/pkg/codingcontext/markdown/markdown.go +++ b/pkg/codingcontext/markdown/markdown.go @@ -3,6 +3,7 @@ package markdown import ( "bytes" "fmt" + "log/slog" "os" "path/filepath" "strings" @@ -68,7 +69,16 @@ type RuleMarkdown = Markdown[RuleFrontMatter] // ParseMarkdownFile parses a markdown file into frontmatter and content using goldmark. // Errors include file path and, where available, line and column position. +// Uses slog.Default() for taskparser WARN logs; for a custom logger use +// ParseMarkdownFileWithLogger. func ParseMarkdownFile[T any](path string, frontMatter *T) (Markdown[T], error) { + return ParseMarkdownFileWithLogger(path, frontMatter, nil) +} + +// ParseMarkdownFileWithLogger is like ParseMarkdownFile but routes taskparser +// best-effort fallback WARN logs through the given logger. A nil logger +// resolves to slog.Default() at parse time (not capture time). +func ParseMarkdownFileWithLogger[T any](path string, frontMatter *T, logger *slog.Logger) (Markdown[T], error) { cleanPath := filepath.Clean(path) source, err := os.ReadFile(cleanPath) @@ -77,9 +87,9 @@ func ParseMarkdownFile[T any](path string, frontMatter *T) (Markdown[T], error) } // Parse with goldmark+meta+taskparser in a single pass: meta extracts frontmatter, - // taskparser.Extension captures task structure (slash commands) from the body. + // the taskparser extension captures task structure (slash commands) from the body. pctx := parser.NewContext() - doc := goldmark.New(goldmark.WithExtensions(meta.Meta, taskparser.Extension)).Parser(). + doc := goldmark.New(goldmark.WithExtensions(meta.Meta, taskparser.NewExtension(logger))).Parser(). Parse(text.NewReader(source), parser.WithContext(pctx)) // Get frontmatter map from goldmark-meta (parsed during goldmark parse). diff --git a/pkg/codingcontext/taskparser/markdownparser.go b/pkg/codingcontext/taskparser/markdownparser.go index 0521f72f..f35301b5 100644 --- a/pkg/codingcontext/taskparser/markdownparser.go +++ b/pkg/codingcontext/taskparser/markdownparser.go @@ -96,14 +96,14 @@ func collectCodeRanges(doc ast.Node) ([]codeRange, error) { // newline characters. This prevents the next text segment from starting with a bare // newline immediately before a slash, which would cause the grammar parser to fail // (it cannot parse a Text block that has only leading newlines before a slash). -func splitAndParse(content string, codeRanges []codeRange) (Task, error) { +func splitAndParse(content string, codeRanges []codeRange, logger *slog.Logger) (Task, error) { var allBlocks []Block pos := 0 for i, cr := range codeRanges { if pos < cr.start { - blocks, err := parseGrammar(content[pos:cr.start]) + blocks, err := parseGrammar(content[pos:cr.start], logger) if err != nil { return nil, err } @@ -128,7 +128,7 @@ func splitAndParse(content string, codeRanges []codeRange) (Task, error) { } if pos < len(content) { - blocks, err := parseGrammar(content[pos:]) + blocks, err := parseGrammar(content[pos:], logger) if err != nil { return nil, err } @@ -154,7 +154,7 @@ func trailingNewlineEnd(content string, pos int) int { // On parser failure (e.g. a line starting with `/` but lacking a valid term: // `//`, lone `/`, `/=foo`), falls back to a best-effort line-by-line parse so // one malformed line cannot discard the entire task. -func parseGrammar(content string) ([]Block, error) { +func parseGrammar(content string, logger *slog.Logger) ([]Block, error) { if strings.TrimSpace(content) == "" { return nil, nil } @@ -164,13 +164,24 @@ func parseGrammar(content string) ([]Block, error) { return input.Blocks, nil } - return parseGrammarBestEffort(content), nil + return parseGrammarBestEffort(content, logger), nil +} + +// loggerOrDefault returns logger, or slog.Default() if logger is nil. Resolving +// at use time (rather than capturing at construction) ensures slog.SetDefault +// changes take effect for callers that pass nil. +func loggerOrDefault(logger *slog.Logger) *slog.Logger { + if logger == nil { + return slog.Default() + } + + return logger } // parseGrammarBestEffort parses content line-by-line, returning a raw text // block for any line the grammar rejects. Called only after the whole-segment // parse has already failed. -func parseGrammarBestEffort(content string) []Block { +func parseGrammarBestEffort(content string, logger *slog.Logger) []Block { var blocks []Block for _, line := range strings.SplitAfter(content, "\n") { @@ -185,7 +196,7 @@ func parseGrammarBestEffort(content string) []Block { input, err := parser().ParseString("", line) if err != nil { - slog.Warn("taskparser: malformed slash command, treating line as text", + loggerOrDefault(logger).Warn("taskparser: malformed slash command, treating line as text", "line", strings.TrimRight(line, "\r\n"), "error", err) blocks = append(blocks, rawTextBlock(line)) @@ -198,7 +209,6 @@ func parseGrammarBestEffort(content string) []Block { return blocks } - // rawTextBlock wraps a raw string as a Text block without any slash command parsing. // Content() and String() on the returned block both return the original string exactly. func rawTextBlock(content string) Block { @@ -213,6 +223,8 @@ func rawTextBlock(content string) Block { // Extension is a goldmark extension that parses task structure during the markdown parse. // Include it in a goldmark instance and use GetTask to retrieve the parsed Task after parsing. +// Uses slog.Default() for WARN logs from the best-effort fallback; for a custom logger +// use NewExtension. // // Example: // @@ -222,14 +234,23 @@ func rawTextBlock(content string) Block { // task, err := taskparser.GetTask(pctx) // //nolint:gochecknoglobals // goldmark.WithExtensions expects a package-level extender -var Extension goldmark.Extender = &taskExtension{} +var Extension goldmark.Extender = NewExtension(nil) -type taskExtension struct{} +// NewExtension returns a goldmark extension that parses task structure and routes +// best-effort fallback WARN logs through the given logger. A nil logger resolves to +// slog.Default() at parse time (not capture time), so SetDefault changes take effect. +func NewExtension(logger *slog.Logger) goldmark.Extender { + return &taskExtension{logger: logger} +} + +type taskExtension struct { + logger *slog.Logger // nil means use slog.Default() at parse time +} func (e *taskExtension) Extend(m goldmark.Markdown) { const taskTransformerPriority = 100 m.Parser().AddOptions(gparser.WithASTTransformers( - util.Prioritized(&taskTransformer{}, taskTransformerPriority), + util.Prioritized(&taskTransformer{logger: e.logger}, taskTransformerPriority), )) } @@ -262,7 +283,9 @@ func GetTask(pc gparser.Context) (Task, error) { // taskTransformer implements parser.ASTTransformer. It runs after goldmark has built // the document AST and extracts task structure (text vs. slash commands), skipping // slash command detection inside code blocks, indented code, and HTML blocks. -type taskTransformer struct{} +type taskTransformer struct { + logger *slog.Logger +} func (t *taskTransformer) Transform(node *ast.Document, reader text.Reader, pc gparser.Context) { source := reader.Source() @@ -302,17 +325,17 @@ func (t *taskTransformer) Transform(node *ast.Document, reader text.Reader, pc g adjusted = append(adjusted, codeRange{adjStart, adjStop}) } - task, parseErr := splitAndParse(content, adjusted) + task, parseErr := splitAndParse(content, adjusted, t.logger) pc.Set(contextKey, &taskParseResult{task: task, err: parseErr}) } // parseMarkdownAware parses task content while skipping slash command detection inside // code blocks (fenced code, indented code, HTML blocks) by running the Extension -// during a single goldmark parse pass. -func parseMarkdownAware(content string) (Task, error) { +// during a single goldmark parse pass. A nil logger falls back to slog.Default(). +func parseMarkdownAware(content string, logger *slog.Logger) (Task, error) { source := []byte(content) pctx := gparser.NewContext() - goldmark.New(goldmark.WithExtensions(Extension)).Parser(). + goldmark.New(goldmark.WithExtensions(NewExtension(logger))).Parser(). Parse(text.NewReader(source), gparser.WithContext(pctx)) return GetTask(pctx) diff --git a/pkg/codingcontext/taskparser/taskparser.go b/pkg/codingcontext/taskparser/taskparser.go index 9ea37abf..faedde38 100644 --- a/pkg/codingcontext/taskparser/taskparser.go +++ b/pkg/codingcontext/taskparser/taskparser.go @@ -2,6 +2,7 @@ package taskparser import ( + "log/slog" "strings" ) @@ -111,13 +112,20 @@ import ( // task, _ := ParseTask("Introduction text\n/fix-bug 123\nSome text after") // // len(task) == 3 (text, command, text) func ParseTask(text string) (Task, error) { + return ParseTaskWithLogger(text, nil) +} + +// ParseTaskWithLogger is like ParseTask but routes best-effort fallback WARN +// logs through the given logger. A nil logger resolves to slog.Default() at +// parse time (not capture time), so SetDefault changes take effect. +func ParseTaskWithLogger(text string, logger *slog.Logger) (Task, error) { // Handle empty or whitespace-only content gracefully // TrimSpace returns empty string for whitespace-only input if strings.TrimSpace(text) == "" { return Task{}, nil } - return parseMarkdownAware(text) + return parseMarkdownAware(text, logger) } // Params converts the slash command's arguments into a parameter map using ParseParams. diff --git a/pkg/codingcontext/taskparser/taskparser_test.go b/pkg/codingcontext/taskparser/taskparser_test.go index 66562d3f..20377618 100644 --- a/pkg/codingcontext/taskparser/taskparser_test.go +++ b/pkg/codingcontext/taskparser/taskparser_test.go @@ -1,6 +1,8 @@ package taskparser import ( + "bytes" + "log/slog" "strings" "testing" ) @@ -528,6 +530,54 @@ func TestParseTask_BestEffortMalformedSlash(t *testing.T) { } } +// Verifies ParseTaskWithLogger routes the best-effort fallback WARN through +// the injected logger rather than slog.Default(), so callers can capture or +// redirect parser warnings. +func TestParseTaskWithLogger_RoutesWarnToInjectedLogger(t *testing.T) { + t.Parallel() + + var buf bytes.Buffer + logger := slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn})) + + input := "Some text\n// stray\nMore text\n" + + if _, err := ParseTaskWithLogger(input, logger); err != nil { + t.Fatalf("ParseTaskWithLogger() error = %v, want nil", err) + } + + out := buf.String() + if !strings.Contains(out, "malformed slash command") { + t.Errorf("expected WARN about malformed slash command in injected logger output, got %q", out) + } + + if !strings.Contains(out, "// stray") { + t.Errorf("expected WARN to include the offending line, got %q", out) + } +} + +// Verifies a nil logger resolves to slog.Default() at parse time, not capture +// time — so SetDefault changes made after the package loads still take effect. +// Guards against a regression where the default is snapshotted at construction. +func TestParseTaskWithLogger_NilLoggerResolvesLazyDefault(t *testing.T) { + // Not t.Parallel: this test mutates slog's package-level default. + originalDefault := slog.Default() + + t.Cleanup(func() { slog.SetDefault(originalDefault) }) + + var buf bytes.Buffer + slog.SetDefault(slog.New(slog.NewTextHandler(&buf, &slog.HandlerOptions{Level: slog.LevelWarn}))) + + input := "Some text\n// stray\nMore text\n" + + if _, err := ParseTaskWithLogger(input, nil); err != nil { + t.Fatalf("ParseTaskWithLogger(nil) error = %v, want nil", err) + } + + if !strings.Contains(buf.String(), "malformed slash command") { + t.Errorf("expected WARN to be routed through the newly-set default logger, got %q", buf.String()) + } +} + func TestParseTask_MarkdownAware(t *testing.T) { t.Parallel()