Skip to content

Commit 6290a7e

Browse files
committed
fix: address code review findings
- document {{TASK_HEADER_PATTERNS}} in CLAUDE.md and README.md variable lists - update CLAUDE.md 'Plan format' key pattern to reflect configurable headers and the removal of the package-level taskHeaderPattern regex - rename TestDefaultPatternsMatchLegacyRegex and rewrite its comment to be honest: the template-driven compiler is stricter than the legacy regex about whitespace, so the test only covers canonical inputs - exclude gosec G704 (SSRF taint) in *_test.go files; false positive on httptest server URLs in pkg/web/watcher_test.go
1 parent 85c256f commit 6290a7e

20 files changed

Lines changed: 980 additions & 129 deletions

.golangci.yml

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,10 @@ linters:
141141
- noctx
142142
- usestdlibvars
143143
path: _test\.go$
144+
- linters:
145+
- gosec
146+
text: "G704: SSRF via taint analysis"
147+
path: _test\.go$
144148
- linters:
145149
- errcheck
146150
text: "Error return value of .*.Close.*is not checked"

CLAUDE.md

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -61,7 +61,7 @@ docs/plans/ # plan files location
6161

6262
## Key Patterns
6363

64-
- Plan format: Checkboxes (`- [ ]` / `- [x]`) belong only in Task sections (`### Task N:` or `### Iteration N:`). The `Task` / `Iteration` keywords are structural tokens matched by `pkg/plan/parse.go` (`taskHeaderPattern`) and MUST stay in English even when plan content is written in another language — task titles and body text may be localized, but the section header keyword is fixed. Success criteria, Overview, and Context should not use checkboxes — they cause extra loop iterations. The task prompt handles them when present, but plan authors should avoid them.
64+
- Plan format: Checkboxes (`- [ ]` / `- [x]`) belong only in Task sections. By default, headers matching `### Task {N}: {title}` or `### Iteration {N}: {title}` are recognized; the `Task` / `Iteration` keywords are structural tokens and MUST stay in English even when plan content is written in another language — task titles and body text may be localized, but the default section header keywords are fixed. Recognized header shapes are configurable via `task_header_patterns` (compiled by `pkg/plan/patterns.go`, passed to `pkg/plan/parse.go` `ParsePlan`/`ParsePlanFile` as a variadic `patterns ...string`; empty falls back to `plan.DefaultTaskHeaderPatterns`). Success criteria, Overview, and Context should not use checkboxes — they cause extra loop iterations. The task prompt handles them when present, but plan authors should avoid them.
6565
- Signal-based completion detection (COMPLETED, FAILED, REVIEW_DONE signals) — constants in `pkg/status/`
6666
- Plan creation signals: QUESTION (with JSON payload) and PLAN_READY
6767
- Streaming output with timestamps
@@ -373,6 +373,7 @@ Implementation:
373373
- `{{DEFAULT_BRANCH}}` - detected default branch (main, master, origin/main, etc.), overridable via `--base-ref` CLI flag or `default_branch` config option
374374
- `{{DIFF_INSTRUCTION}}` - git diff command for current iteration (first: `git diff main...HEAD`, subsequent: `git diff`)
375375
- `{{PREVIOUS_REVIEW_CONTEXT}}` - previous review context block for external review iterations (empty on first iteration, formatted context on subsequent)
376+
- `{{TASK_HEADER_PATTERNS}}` - quoted, `or`-joined list of configured `task_header_patterns` templates (used in `task.txt`)
376377
- `{{agent:name}}` - expands to Task tool instructions for the named agent
377378

378379
Variables are also expanded inside agent content, so custom agents can use `{{DEFAULT_BRANCH}}` etc.

README.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -692,6 +692,7 @@ Custom prompt files support variable expansion. All variables use the `{{VARIABL
692692
| `{{PROGRESS_FILE}}` | Path to the progress log file | `.ralphex/progress/progress-feature.txt` |
693693
| `{{GOAL}}` | Human-readable goal description | `implementation of plan at docs/plans/feature.md` |
694694
| `{{DEFAULT_BRANCH}}` | Default branch name (overridable via `--base-ref` or `default_branch` config) | `main`, `master`, `origin/main` |
695+
| `{{TASK_HEADER_PATTERNS}}` | Quoted, `or`-joined list of configured `task_header_patterns` templates (used in `task.txt`) | `'### Task {N}: {title}' or '### Iteration {N}: {title}'` |
695696
| `{{agent:name}}` | Expands to Task tool instructions for the named agent | (see below) |
696697

697698
**Agent references:**

cmd/ralphex/main.go

Lines changed: 32 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -399,11 +399,16 @@ func setupProgressLogger(o opts, req executePlanRequest, branch string) (progres
399399
baseLog = req.ProgressLog
400400
} else {
401401
var err error
402+
var headerPatterns []string
403+
if req.Config != nil {
404+
headerPatterns = req.Config.TaskHeaderPatterns
405+
}
402406
baseLog, err = progress.NewLogger(progress.Config{
403-
PlanFile: req.PlanFile,
404-
Mode: string(req.Mode),
405-
Branch: branch,
406-
NoColor: o.NoColor,
407+
PlanFile: req.PlanFile,
408+
Mode: string(req.Mode),
409+
Branch: branch,
410+
NoColor: o.NoColor,
411+
TaskHeaderPatterns: headerPatterns,
407412
}, req.Colors, holder)
408413
if err != nil {
409414
return progressLogResult{}, fmt.Errorf("create progress logger: %w", err)
@@ -514,14 +519,15 @@ func executePlan(ctx context.Context, o opts, req executePlanRequest) error {
514519
var runnerLog processor.Logger = plr.baseLog
515520
if o.Serve {
516521
dashboard := web.NewDashboard(web.DashboardConfig{
517-
BaseLog: plr.baseLog,
518-
Port: o.Port,
519-
Host: o.Host,
520-
PlanFile: req.PlanFile,
521-
Branch: branch,
522-
WatchDirs: o.Watch,
523-
ConfigWatchDirs: req.Config.WatchDirs,
524-
Colors: req.Colors,
522+
BaseLog: plr.baseLog,
523+
Port: o.Port,
524+
Host: o.Host,
525+
PlanFile: req.PlanFile,
526+
Branch: branch,
527+
WatchDirs: o.Watch,
528+
ConfigWatchDirs: req.Config.WatchDirs,
529+
Colors: req.Colors,
530+
TaskHeaderPatterns: req.Config.TaskHeaderPatterns,
525531
}, plr.holder)
526532
var dashErr error
527533
runnerLog, dashErr = dashboard.Start(ctx)
@@ -646,10 +652,11 @@ func runWithWorktree(ctx context.Context, o opts, req executePlanRequest) (err e
646652
holder := &status.PhaseHolder{}
647653
branch := plan.ExtractBranchName(req.PlanFile)
648654
baseLog, err := progress.NewLogger(progress.Config{
649-
PlanFile: req.PlanFile,
650-
Mode: string(req.Mode),
651-
Branch: branch,
652-
NoColor: o.NoColor,
655+
PlanFile: req.PlanFile,
656+
Mode: string(req.Mode),
657+
Branch: branch,
658+
NoColor: o.NoColor,
659+
TaskHeaderPatterns: req.Config.TaskHeaderPatterns,
653660
}, req.Colors, holder)
654661
if err != nil {
655662
return fmt.Errorf("create progress logger: %w", err)
@@ -767,9 +774,10 @@ func isWatchOnlyMode(o opts, configWatchDirs []string) bool {
767774
func runWatchOnly(ctx context.Context, o opts, cfg *config.Config, colors *progress.Colors) error {
768775
dirs := web.ResolveWatchDirs(o.Watch, cfg.WatchDirs)
769776
dashboard := web.NewDashboard(web.DashboardConfig{
770-
Port: o.Port,
771-
Host: o.Host,
772-
Colors: colors,
777+
Port: o.Port,
778+
Host: o.Host,
779+
Colors: colors,
780+
TaskHeaderPatterns: cfg.TaskHeaderPatterns,
773781
}, nil)
774782
if watchErr := dashboard.RunWatchOnly(ctx, dirs); watchErr != nil {
775783
return fmt.Errorf("run watch-only mode: %w", watchErr)
@@ -934,10 +942,11 @@ func runPlanMode(ctx context.Context, o opts, req executePlanRequest, selector *
934942

935943
// create progress logger for plan mode
936944
baseLog, err := progress.NewLogger(progress.Config{
937-
PlanDescription: o.PlanDescription,
938-
Mode: string(processor.ModePlan),
939-
Branch: branch,
940-
NoColor: o.NoColor,
945+
PlanDescription: o.PlanDescription,
946+
Mode: string(processor.ModePlan),
947+
Branch: branch,
948+
NoColor: o.NoColor,
949+
TaskHeaderPatterns: req.Config.TaskHeaderPatterns,
941950
}, req.Colors, holder)
942951
if err != nil {
943952
return fmt.Errorf("create progress logger: %w", err)

pkg/plan/parse.go

Lines changed: 54 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -71,6 +71,37 @@ func matchTaskHeader(line string, compiled []*regexp.Regexp) (taskID, title stri
7171
return "", "", false
7272
}
7373

74+
// headingLevel returns the number of leading '#' characters on a line, or 0 if
75+
// the line is not a markdown heading. a line starting with '#' is considered a
76+
// heading regardless of whether a space follows (matching the legacy behavior
77+
// that used strings.HasPrefix without whitespace checks).
78+
func headingLevel(line string) int {
79+
i := 0
80+
for i < len(line) && line[i] == '#' {
81+
i++
82+
}
83+
return i
84+
}
85+
86+
// closesTask reports whether a non-task-matching heading at lineLevel should
87+
// close a task opened at taskLevel. strictly shallower headings always close
88+
// (they start a new top-level section). same-level headings close only when
89+
// the task lives at the top of the document tree (level 1 or 2), because at
90+
// those levels a same-level heading is a sibling section, not a sub-note; at
91+
// deeper levels a same-level non-matching heading is treated as a note inside
92+
// the task so its checkboxes remain attached. see parse_test.go cases
93+
// "non-matching h3 does NOT close current task" and
94+
// "H1 task template closes preceding task on later non-task H1".
95+
func closesTask(lineLevel, taskLevel int) bool {
96+
if lineLevel <= 0 || taskLevel <= 0 {
97+
return false
98+
}
99+
if lineLevel < taskLevel {
100+
return true
101+
}
102+
return lineLevel == taskLevel && taskLevel <= 2
103+
}
104+
74105
// ParsePlan parses plan markdown content into a structured Plan.
75106
// patterns is an optional variadic list of task-header templates (e.g.
76107
// "### Task {N}: {title}"). If empty, DefaultTaskHeaderPatterns is used.
@@ -86,19 +117,15 @@ func ParsePlan(content string, patterns ...string) (*Plan, error) {
86117

87118
scanner := bufio.NewScanner(strings.NewReader(content))
88119
var currentTask *Task
120+
currentTaskLevel := 0 // heading level (# count) of the currently open task
89121

90122
for scanner.Scan() {
91123
line := scanner.Text()
124+
level := headingLevel(line)
92125

93-
// check for plan title (first h1)
94-
if p.Title == "" {
95-
if matches := titlePattern.FindStringSubmatch(line); matches != nil {
96-
p.Title = strings.TrimSpace(matches[1])
97-
continue
98-
}
99-
}
100-
101-
// check for task header (first match wins across configured patterns)
126+
// check for task header first (first match wins across configured patterns).
127+
// runs before the H1 title capture so a custom H1 task template like
128+
// "# {N}. {title}" is not silently consumed as the plan title.
102129
if id, title, matched := matchTaskHeader(line, compiled); matched {
103130
// save previous task if exists
104131
if currentTask != nil {
@@ -112,22 +139,33 @@ func ParsePlan(content string, patterns ...string) (*Plan, error) {
112139
Status: TaskStatusPending,
113140
Checkboxes: make([]Checkbox, 0),
114141
}
142+
currentTaskLevel = level
115143
continue
116144
}
117145

118-
// non-Task section header (e.g. ## Success criteria, ## Overview, ## Context):
119-
// close current task so checkboxes below are not attached to it.
120-
// only ## (h2) closes; ### and #### are subsections and must not orphan checkboxes.
121-
// also close on # (h1) when title already set, e.g. # Overview in plans using single hash for sections.
122-
isH2 := strings.HasPrefix(line, "##") && !strings.HasPrefix(line, "###")
123-
isH1AfterTitle := strings.HasPrefix(line, "#") && p.Title != "" && !strings.HasPrefix(line, "##")
124-
if currentTask != nil && (isH2 || isH1AfterTitle) {
146+
// non-task heading: close the current task when it starts a new section at
147+
// a shallower or sibling-top-level position (see closesTask). this lets
148+
// deeper headings (e.g. #### inside a ### task) stay attached to the task
149+
// as sub-notes, while a ## Success criteria or # Overview still closes a
150+
// ### task, and a ## Phase task is not prematurely closed by a ### note.
151+
if currentTask != nil && closesTask(level, currentTaskLevel) {
125152
currentTask.Status = DetermineTaskStatus(currentTask.Checkboxes)
126153
p.Tasks = append(p.Tasks, *currentTask)
127154
currentTask = nil
155+
currentTaskLevel = 0
128156
continue
129157
}
130158

159+
// check for plan title (first h1) — only when no task header matched above
160+
// and no task is open, so H1-style task templates aren't swallowed here and
161+
// a later # Section doesn't retroactively become the plan title.
162+
if p.Title == "" && level == 1 {
163+
if matches := titlePattern.FindStringSubmatch(line); matches != nil {
164+
p.Title = strings.TrimSpace(matches[1])
165+
continue
166+
}
167+
}
168+
131169
// check for checkbox (only if inside a task)
132170
if currentTask != nil {
133171
if matches := checkboxPattern.FindStringSubmatch(line); matches != nil {

pkg/plan/parse_test.go

Lines changed: 136 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -516,6 +516,142 @@ also no checkboxes.
516516
assert.Equal(t, plan.TaskStatusPending, p.Tasks[0].Status)
517517
})
518518

519+
t.Run("H1 task template does not lose first task to title capture", func(t *testing.T) {
520+
// custom template using a single hash header; the first line must be
521+
// captured as a task, not silently consumed as the plan title.
522+
content := `# 1. First Phase
523+
524+
- [ ] first item
525+
526+
# 2. Second Phase
527+
528+
- [ ] second item
529+
`
530+
p, err := plan.ParsePlan(content, "# {N}. {title}")
531+
require.NoError(t, err)
532+
assert.Empty(t, p.Title, "first H1 must not be consumed as plan title when it matches a task template")
533+
require.Len(t, p.Tasks, 2)
534+
assert.Equal(t, 1, p.Tasks[0].Number)
535+
assert.Equal(t, "First Phase", p.Tasks[0].Title)
536+
require.Len(t, p.Tasks[0].Checkboxes, 1)
537+
assert.Equal(t, 2, p.Tasks[1].Number)
538+
assert.Equal(t, "Second Phase", p.Tasks[1].Title)
539+
})
540+
541+
t.Run("H1 task template closes preceding task on later non-task H1", func(t *testing.T) {
542+
// with a custom H1 task template and no separate plan title,
543+
// a later non-task "# Section" must close the current task
544+
// instead of being swallowed as the plan title while checkboxes
545+
// below silently attach to the preceding task.
546+
content := `# 1. First Phase
547+
548+
- [ ] first item
549+
550+
# Overview
551+
552+
- [ ] outside
553+
`
554+
p, err := plan.ParsePlan(content, "# {N}. {title}")
555+
require.NoError(t, err)
556+
require.Len(t, p.Tasks, 1)
557+
assert.Equal(t, 1, p.Tasks[0].Number)
558+
assert.Equal(t, "First Phase", p.Tasks[0].Title)
559+
require.Len(t, p.Tasks[0].Checkboxes, 1)
560+
assert.Equal(t, "first item", p.Tasks[0].Checkboxes[0].Text)
561+
})
562+
563+
t.Run("default patterns still capture H1 plan title", func(t *testing.T) {
564+
// default templates (### Task/Iteration) do not match "# Title", so the
565+
// first H1 must still be captured as the plan title.
566+
content := `# Plan Title
567+
568+
### Task 1: Do Stuff
569+
570+
- [ ] item
571+
`
572+
p, err := plan.ParsePlan(content)
573+
require.NoError(t, err)
574+
assert.Equal(t, "Plan Title", p.Title)
575+
require.Len(t, p.Tasks, 1)
576+
assert.Equal(t, "Do Stuff", p.Tasks[0].Title)
577+
})
578+
579+
t.Run("H1 task template: later ## subsection does NOT close task", func(t *testing.T) {
580+
// with H1-level task headers, a ## subsection inside the task must remain
581+
// attached (it's deeper than the task heading, so it's a sub-note).
582+
content := `# 1. First Phase
583+
584+
- [ ] main item
585+
586+
## Details
587+
588+
- [ ] sub item
589+
590+
# 2. Second Phase
591+
592+
- [ ] second main
593+
`
594+
p, err := plan.ParsePlan(content, "# {N}. {title}")
595+
require.NoError(t, err)
596+
597+
require.Len(t, p.Tasks, 2)
598+
assert.Equal(t, 1, p.Tasks[0].Number)
599+
require.Len(t, p.Tasks[0].Checkboxes, 2)
600+
assert.Equal(t, "main item", p.Tasks[0].Checkboxes[0].Text)
601+
assert.Equal(t, "sub item", p.Tasks[0].Checkboxes[1].Text)
602+
603+
assert.Equal(t, 2, p.Tasks[1].Number)
604+
require.Len(t, p.Tasks[1].Checkboxes, 1)
605+
})
606+
607+
t.Run("H4 task template: higher-level ### section closes task", func(t *testing.T) {
608+
// with H4-level task headers (deeper than the default), a shallower ###
609+
// section must still close the task so its checkboxes don't leak.
610+
content := `# Plan
611+
612+
#### 1. Deep Task
613+
614+
- [ ] item inside task
615+
616+
### Sibling Section
617+
618+
- [ ] outside task
619+
`
620+
p, err := plan.ParsePlan(content, "#### {N}. {title}")
621+
require.NoError(t, err)
622+
623+
require.Len(t, p.Tasks, 1)
624+
require.Len(t, p.Tasks[0].Checkboxes, 1)
625+
assert.Equal(t, "item inside task", p.Tasks[0].Checkboxes[0].Text)
626+
})
627+
628+
t.Run("H2 task template: ### sub-note stays inside task", func(t *testing.T) {
629+
// for a ## task, a deeper ### heading is a sub-note and must NOT close
630+
// the task (regression guard: any sibling-same-level logic must not fire here).
631+
content := `# Plan
632+
633+
## 1. Phase One
634+
635+
- [ ] main item
636+
637+
### Notes
638+
639+
- [ ] still inside task
640+
641+
## 2. Phase Two
642+
643+
- [ ] second
644+
`
645+
p, err := plan.ParsePlan(content, "## {N}. {title}")
646+
require.NoError(t, err)
647+
648+
require.Len(t, p.Tasks, 2)
649+
require.Len(t, p.Tasks[0].Checkboxes, 2)
650+
assert.Equal(t, "main item", p.Tasks[0].Checkboxes[0].Text)
651+
assert.Equal(t, "still inside task", p.Tasks[0].Checkboxes[1].Text)
652+
require.Len(t, p.Tasks[1].Checkboxes, 1)
653+
})
654+
519655
t.Run("ParsePlanFile accepts patterns variadic", func(t *testing.T) {
520656
content := `# Plan
521657

0 commit comments

Comments
 (0)