From d0cb9e32c5cd9c93415ae1f25bc921e887998ffe Mon Sep 17 00:00:00 2001 From: Umputun Date: Tue, 12 May 2026 13:48:29 -0500 Subject: [PATCH 1/5] feat: drop redundant move-plan checkbox from make_plan.txt The framework already moves the plan to completed/ at end of run via MovePlanToCompleted, gated by the move_plan_on_completion config option. The LLM-task move was redundant and the source of mid-run rename risk when the model wrote a different basename than the file on disk. Add a tripwire test in pkg/config/prompts_test.go that asserts the embedded template no longer contains any path-relocation substring (move this plan, completed/, mv, relocate), catching reworded re-introductions, not just the verbatim original. Related to umputun/ralphex#341 --- pkg/config/defaults/prompts/make_plan.txt | 1 - pkg/config/prompts_test.go | 19 +++++++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/pkg/config/defaults/prompts/make_plan.txt b/pkg/config/defaults/prompts/make_plan.txt index 5da1dbdf..ced970c5 100644 --- a/pkg/config/defaults/prompts/make_plan.txt +++ b/pkg/config/defaults/prompts/make_plan.txt @@ -153,7 +153,6 @@ Write the accepted plan to disk: - [ ] update README.md if user-facing changes - [ ] update CLAUDE.md if internal patterns changed -- [ ] move this plan to `docs/plans/completed/` --- ## Step 4.5: Validate Plan Before Draft diff --git a/pkg/config/prompts_test.go b/pkg/config/prompts_test.go index 56c625cb..10657fdc 100644 --- a/pkg/config/prompts_test.go +++ b/pkg/config/prompts_test.go @@ -784,3 +784,22 @@ func TestPromptLoader_Load_CustomEvalPrompt_LocalOverridesGlobal(t *testing.T) { assert.Equal(t, "local custom eval", prompts.CustomEval) } + +// tripwire regression test: the embedded make_plan.txt must not instruct claude to relocate +// the plan file at end of run. the framework's MovePlanToCompleted (pkg/git/service.go) already +// handles that move idempotently using the resolved plan-file basename, so any LLM-driven git mv +// can rename the file out from under the runtime and trigger the infinite warning loop described +// in umputun/ralphex#341. if a template change legitimately re-introduces an LLM-driven plan-file +// relocation, this test must be updated deliberately, not silently. +func TestMakePlanPrompt_NoLLMDrivenPlanMove(t *testing.T) { + loader := newPromptLoader(defaultsFS) + content, err := loader.loadPromptFromEmbedFS("defaults/prompts/make_plan.txt") + require.NoError(t, err) + require.NotEmpty(t, content) + + forbidden := []string{"move this plan", "completed/", "mv ", "relocate"} + for _, sub := range forbidden { + assert.NotContains(t, content, sub, + "embedded make_plan.txt must not contain %q (would re-introduce LLM-driven plan-file relocation)", sub) + } +} From 5defc4aeb69c16dc31c896b2965327eeb7161464 Mon Sep 17 00:00:00 2001 From: Umputun Date: Tue, 12 May 2026 13:48:46 -0500 Subject: [PATCH 2/5] feat: tolerate renamed plan file during execution When an LLM-driven task renames the plan file mid-run (e.g. dashed YYYY-MM-DD-.md becomes compact YYYYMMDD-.md after a git mv), the task loop used to spin forever: resolvePlanFilePath fell back to completed/ only, hasUncompletedTasks returned true on every ParsePlanFile error including fs.ErrNotExist, and MovePlanToCompleted's idempotency check missed the renamed destination. Three layers of defense, all using the same dashed/compact swap: - resolvePlanFilePath probes the alternate-format basename in both the original directory (in-place rename) and completed/ (moved and renamed) after the existing exact-basename probes. - hasUncompletedTasks distinguishes fs.ErrNotExist from other parse errors and returns false when the file is truly gone, letting the ALL_TASKS_DONE signal exit the loop. Last-line-of- defense, runs after resolvePlanFilePath has exhausted all probes. - MovePlanToCompleted treats a renamed destination as already-moved for idempotency. When the in-place renamed source collides with a stale completed/ copy, surface as already-completed and leave both files for manual resolution rather than clobbering. - pkg/web/loadPlanWithFallback gets the same alt-format fallback so dashboard plan loading recovers from the same rename. altDateFormatBasename on *Service and tryAlternateDateFormat on *Runner are pure string transformations with no I/O. Related to umputun/ralphex#341 --- pkg/git/service.go | 108 +++++++++++++-- pkg/git/service_test.go | 242 ++++++++++++++++++++++++++++++++++ pkg/processor/prompts.go | 58 +++++++- pkg/processor/prompts_test.go | 123 +++++++++++++++++ pkg/processor/runner.go | 10 ++ pkg/processor/runner_test.go | 25 ++++ pkg/web/plan.go | 55 +++++++- pkg/web/server_test.go | 65 +++++++++ 8 files changed, 666 insertions(+), 20 deletions(-) diff --git a/pkg/git/service.go b/pkg/git/service.go index c74f7d86..8c3e1388 100644 --- a/pkg/git/service.go +++ b/pkg/git/service.go @@ -6,11 +6,22 @@ import ( "io" "os" "path/filepath" + "regexp" "strings" "github.com/umputun/ralphex/pkg/plan" ) +// dashedDatePattern matches YYYY-MM-DD-.md basenames (dashed convention). +// intentionally duplicated from pkg/processor/prompts.go: pkg/git is a low-level +// leaf used by pkg/processor, so it should not import the higher-level package +// just to share a ~10-line helper. +var dashedDatePattern = regexp.MustCompile(`^(\d{4})-(\d{2})-(\d{2})-(.+\.md)$`) + +// compactDatePattern matches YYYYMMDD-.md basenames (compact convention). +// see dashedDatePattern for the duplication rationale. +var compactDatePattern = regexp.MustCompile(`^(\d{8})-(.+\.md)$`) + //go:generate moq -out mocks/logger.go -pkg mocks -skip-ensure -fmt goimports . Logger // Logger provides logging for git operations output. @@ -82,6 +93,20 @@ func (s *Service) SetCommitTrailer(trailer string) { s.trailer = trailer } +// altDateFormatBasename returns the basename with the date prefix swapped between +// dashed (YYYY-MM-DD) and compact (YYYYMMDD) conventions, or "" if name matches +// neither pattern. Pure string transformation, no I/O. +func (s *Service) altDateFormatBasename(name string) string { + if m := dashedDatePattern.FindStringSubmatch(name); m != nil { + return m[1] + m[2] + m[3] + "-" + m[4] + } + if m := compactDatePattern.FindStringSubmatch(name); m != nil { + d := m[1] + return d[0:4] + "-" + d[4:6] + "-" + d[6:8] + "-" + m[2] + } + return "" +} + // appendTrailer appends the configured trailer to a commit message. // returns the message unchanged when no trailer is configured. func (s *Service) appendTrailer(msg string) string { @@ -448,6 +473,14 @@ func (s *Service) RemoveWorktree(path string) error { // Creates the completed/ directory if it doesn't exist. // Uses git mv if the file is tracked, falls back to os.Rename for untracked files. // If the source file doesn't exist but the destination does, logs a message and returns nil. +// Also returns nil when the source is missing and a basename with the alternate date-prefix +// convention (YYYY-MM-DD ↔ YYYYMMDD) exists in completed/, treating an LLM-driven +// date-format rename as already-moved. +// When the source is missing but a file with the alternate date-prefix exists alongside it +// (in-place rename, e.g. git mv 2026-05-12-foo.md 20260512-foo.md), the renamed file is +// used as the source so the move can complete with its current name. If an alternate-date +// copy already exists at the destination (e.g. from a prior same-day run with the same slug), +// the move is skipped so neither file is clobbered. func (s *Service) MovePlanToCompleted(planFile string) error { // create completed directory completedDir := filepath.Join(filepath.Dir(planFile), "completed") @@ -455,21 +488,15 @@ func (s *Service) MovePlanToCompleted(planFile string) error { return fmt.Errorf("create completed dir: %w", err) } - // destination path - destPath := filepath.Join(completedDir, filepath.Base(planFile)) - - // check if already moved (source missing, dest exists) - if _, err := os.Stat(planFile); os.IsNotExist(err) { - if _, destErr := os.Stat(destPath); destErr == nil { - s.log.Printf("plan already in completed/\n") - return nil - } + sourceFile, destPath, done := s.resolvePlanMoveTargets(planFile, completedDir) + if done { + return nil } // use git mv - if err := s.repo.moveFile(planFile, destPath); err != nil { + if err := s.repo.moveFile(sourceFile, destPath); err != nil { // fallback to regular move for untracked files - if renameErr := os.Rename(planFile, destPath); renameErr != nil { + if renameErr := os.Rename(sourceFile, destPath); renameErr != nil { return fmt.Errorf("move plan: %w", renameErr) } // stage the new location - log if fails but continue @@ -479,7 +506,7 @@ func (s *Service) MovePlanToCompleted(planFile string) error { } // commit the move - commitMsg := "move completed plan: " + filepath.Base(planFile) + commitMsg := "move completed plan: " + filepath.Base(sourceFile) if err := s.repo.commit(s.appendTrailer(commitMsg)); err != nil { return fmt.Errorf("commit plan move: %w", err) } @@ -488,6 +515,63 @@ func (s *Service) MovePlanToCompleted(planFile string) error { return nil } +// resolvePlanMoveTargets determines the source and destination for MovePlanToCompleted, +// accounting for files already moved to completed/ or renamed between the dashed +// (YYYY-MM-DD) and compact (YYYYMMDD) date-prefix conventions. Returns done=true when +// the file is already in completed/ (with either basename) and no move is needed. +// Probe order mirrors resolvePlanFilePath in pkg/processor/prompts.go: the in-place +// alternate source is checked before any completed/ probe so a current renamed file +// wins over a stale completed/ copy left from a prior run. +func (s *Service) resolvePlanMoveTargets(planFile, completedDir string) (sourceFile, destPath string, done bool) { + destPath = filepath.Join(completedDir, filepath.Base(planFile)) + sourceFile = planFile + + if _, err := os.Stat(planFile); !os.IsNotExist(err) { + return sourceFile, destPath, false + } + + altBase := s.altDateFormatBasename(filepath.Base(planFile)) + + // file may have been renamed in place (same dir, alt basename) — use it as source. + // checked before completed/ probes so a current renamed file wins over a stale + // completed/ copy from a prior run. + if altBase != "" { + altSourcePath := filepath.Join(filepath.Dir(planFile), altBase) + if _, altSrcErr := os.Stat(altSourcePath); altSrcErr == nil { + altDestPath := filepath.Join(completedDir, altBase) + // collision: a stale completed/ exists alongside the active in-place + // renamed source (e.g. same slug ran twice on the same day). git mv would refuse + // because dest exists, and the os.Rename fallback would clobber the stale copy + // while leaving the source's deletion unstaged — repo ends up dirty or commit + // fails entirely. surface as already-completed instead and preserve both files + // for manual resolution. + if _, altDestErr := os.Stat(altDestPath); altDestErr == nil { + s.log.Printf("plan already in completed/ (renamed: %s); active copy at %s left in place for manual cleanup\n", + altBase, altSourcePath) + return altSourcePath, altDestPath, true + } + return altSourcePath, altDestPath, false + } + } + + if _, destErr := os.Stat(destPath); destErr == nil { + s.log.Printf("plan already in completed/\n") + return sourceFile, destPath, true + } + + if altBase == "" { + return sourceFile, destPath, false + } + + altDestPath := filepath.Join(completedDir, altBase) + if _, altErr := os.Stat(altDestPath); altErr == nil { + s.log.Printf("plan already in completed/ (renamed: %s)\n", altBase) + return sourceFile, destPath, true + } + + return sourceFile, destPath, false +} + // EnsureHasCommits checks that the repository has at least one commit. // If the repository is empty, calls promptFn to ask user whether to create initial commit. // promptFn should return true to create the commit, false to abort. diff --git a/pkg/git/service_test.go b/pkg/git/service_test.go index df4703a1..f6e01f0a 100644 --- a/pkg/git/service_test.go +++ b/pkg/git/service_test.go @@ -554,6 +554,223 @@ func TestService_MovePlanToCompleted(t *testing.T) { require.Len(t, log.logs, 1) assert.Contains(t, log.logs[0], "already in completed") }) + + t.Run("returns nil if renamed to compact date in completed", func(t *testing.T) { + dir := setupExternalTestRepo(t) + log := &mockLogger{} + svc, err := NewService(dir, log) + require.NoError(t, err) + + // simulate prior move that also renamed dashed → compact date prefix + plansDir := filepath.Join(dir, "docs", "plans") + completedDir := filepath.Join(plansDir, "completed") + require.NoError(t, os.MkdirAll(completedDir, 0o750)) + completedPath := filepath.Join(completedDir, "20260512-foo.md") + require.NoError(t, os.WriteFile(completedPath, []byte("# Plan"), 0o600)) + + // caller still references the original dashed-format path + planFile := filepath.Join(plansDir, "2026-05-12-foo.md") + _, err = os.Stat(planFile) + require.True(t, os.IsNotExist(err)) + + err = svc.MovePlanToCompleted(planFile) + require.NoError(t, err) + + require.Len(t, log.logs, 1) + assert.Contains(t, log.logs[0], "already in completed") + assert.Contains(t, log.logs[0], "renamed") + assert.Contains(t, log.logs[0], "20260512-foo.md") + }) + + t.Run("returns nil if renamed to dashed date in completed", func(t *testing.T) { + dir := setupExternalTestRepo(t) + log := &mockLogger{} + svc, err := NewService(dir, log) + require.NoError(t, err) + + // simulate prior move that also renamed compact → dashed date prefix + plansDir := filepath.Join(dir, "docs", "plans") + completedDir := filepath.Join(plansDir, "completed") + require.NoError(t, os.MkdirAll(completedDir, 0o750)) + completedPath := filepath.Join(completedDir, "2026-05-12-foo.md") + require.NoError(t, os.WriteFile(completedPath, []byte("# Plan"), 0o600)) + + // caller still references the original compact-format path + planFile := filepath.Join(plansDir, "20260512-foo.md") + _, err = os.Stat(planFile) + require.True(t, os.IsNotExist(err)) + + err = svc.MovePlanToCompleted(planFile) + require.NoError(t, err) + + require.Len(t, log.logs, 1) + assert.Contains(t, log.logs[0], "already in completed") + assert.Contains(t, log.logs[0], "renamed") + assert.Contains(t, log.logs[0], "2026-05-12-foo.md") + }) + + t.Run("moves file renamed in place to compact date", func(t *testing.T) { + // caller references original dashed path, file was renamed in place to compact + // e.g. git mv docs/plans/2026-05-12-foo.md docs/plans/20260512-foo.md + dir := setupExternalTestRepo(t) + svc, err := NewService(dir, noopServiceLogger()) + require.NoError(t, err) + + plansDir := filepath.Join(dir, "docs", "plans") + require.NoError(t, os.MkdirAll(plansDir, 0o750)) + renamedPath := filepath.Join(plansDir, "20260512-foo.md") + require.NoError(t, os.WriteFile(renamedPath, []byte("# Plan"), 0o600)) + require.NoError(t, svc.repo.add(renamedPath)) + require.NoError(t, svc.repo.commit("add plan with renamed basename")) + + // caller still passes the original dashed path + planFile := filepath.Join(plansDir, "2026-05-12-foo.md") + _, err = os.Stat(planFile) + require.True(t, os.IsNotExist(err)) + + err = svc.MovePlanToCompleted(planFile) + require.NoError(t, err) + + // renamed source should be gone + _, err = os.Stat(renamedPath) + assert.True(t, os.IsNotExist(err)) + + // destination uses the renamed basename + completedPath := filepath.Join(plansDir, "completed", "20260512-foo.md") + _, err = os.Stat(completedPath) + require.NoError(t, err) + }) + + t.Run("moves file renamed in place to dashed date", func(t *testing.T) { + // mirror: caller references compact, file renamed in place to dashed + dir := setupExternalTestRepo(t) + svc, err := NewService(dir, noopServiceLogger()) + require.NoError(t, err) + + plansDir := filepath.Join(dir, "docs", "plans") + require.NoError(t, os.MkdirAll(plansDir, 0o750)) + renamedPath := filepath.Join(plansDir, "2026-05-12-foo.md") + require.NoError(t, os.WriteFile(renamedPath, []byte("# Plan"), 0o600)) + require.NoError(t, svc.repo.add(renamedPath)) + require.NoError(t, svc.repo.commit("add plan with renamed basename")) + + planFile := filepath.Join(plansDir, "20260512-foo.md") + _, err = os.Stat(planFile) + require.True(t, os.IsNotExist(err)) + + err = svc.MovePlanToCompleted(planFile) + require.NoError(t, err) + + _, err = os.Stat(renamedPath) + assert.True(t, os.IsNotExist(err)) + + completedPath := filepath.Join(plansDir, "completed", "2026-05-12-foo.md") + _, err = os.Stat(completedPath) + require.NoError(t, err) + }) + + t.Run("in-place rename wins over stale completed copy", func(t *testing.T) { + // caller passes original dashed path. file was renamed in place to compact AND a stale + // completed/ exists from a prior run. the in-place rename is the + // active plan and must be moved; the stale completed/ copy must not short-circuit. + dir := setupExternalTestRepo(t) + svc, err := NewService(dir, noopServiceLogger()) + require.NoError(t, err) + + plansDir := filepath.Join(dir, "docs", "plans") + require.NoError(t, os.MkdirAll(plansDir, 0o750)) + + // active plan at compact basename (renamed in place) + renamedPath := filepath.Join(plansDir, "20260512-foo.md") + require.NoError(t, os.WriteFile(renamedPath, []byte("# Plan (current)"), 0o600)) + require.NoError(t, svc.repo.add(renamedPath)) + require.NoError(t, svc.repo.commit("add plan with renamed basename")) + + // stale completed copy at original (dashed) basename from a prior run + completedDir := filepath.Join(plansDir, "completed") + require.NoError(t, os.MkdirAll(completedDir, 0o750)) + stalePath := filepath.Join(completedDir, "2026-05-12-foo.md") + require.NoError(t, os.WriteFile(stalePath, []byte("# Plan (stale)"), 0o600)) + + // caller still references the original dashed path + planFile := filepath.Join(plansDir, "2026-05-12-foo.md") + _, err = os.Stat(planFile) + require.True(t, os.IsNotExist(err)) + + err = svc.MovePlanToCompleted(planFile) + require.NoError(t, err) + + // renamed source should be gone (was moved, not abandoned) + _, err = os.Stat(renamedPath) + assert.True(t, os.IsNotExist(err), "active in-place renamed file should have been moved") + + // destination uses the renamed basename + movedPath := filepath.Join(completedDir, "20260512-foo.md") + movedContent, err := os.ReadFile(movedPath) //nolint:gosec // test file + require.NoError(t, err) + assert.Equal(t, "# Plan (current)", string(movedContent), "moved file should contain current content") + + // stale completed copy is left in place (not our responsibility to clean up) + staleContent, err := os.ReadFile(stalePath) //nolint:gosec // test file + require.NoError(t, err) + assert.Equal(t, "# Plan (stale)", string(staleContent)) + }) + + t.Run("collision between in-place rename and stale completed/ is left untouched", func(t *testing.T) { + // caller passes original dashed path. file was renamed in place to compact AND a stale + // completed/ copy already exists with the same basename (e.g. same slug ran + // twice on the same day). git mv would refuse, os.Rename fallback would clobber the + // stale copy and leave the source's deletion unstaged. verify we surface this as + // already-completed and preserve both files for manual resolution. + dir := setupExternalTestRepo(t) + log := &mockLogger{} + svc, err := NewService(dir, log) + require.NoError(t, err) + + plansDir := filepath.Join(dir, "docs", "plans") + require.NoError(t, os.MkdirAll(plansDir, 0o750)) + + // active plan at compact basename (renamed in place, tracked) + renamedPath := filepath.Join(plansDir, "20260512-foo.md") + require.NoError(t, os.WriteFile(renamedPath, []byte("# Plan (current)"), 0o600)) + require.NoError(t, svc.repo.add(renamedPath)) + require.NoError(t, svc.repo.commit("add plan with renamed basename")) + + // stale completed copy at compact basename from a prior run with same slug+date + completedDir := filepath.Join(plansDir, "completed") + require.NoError(t, os.MkdirAll(completedDir, 0o750)) + stalePath := filepath.Join(completedDir, "20260512-foo.md") + require.NoError(t, os.WriteFile(stalePath, []byte("# Plan (stale)"), 0o600)) + + // caller references the original dashed path + planFile := filepath.Join(plansDir, "2026-05-12-foo.md") + _, err = os.Stat(planFile) + require.True(t, os.IsNotExist(err)) + + err = svc.MovePlanToCompleted(planFile) + require.NoError(t, err) + + // active source must be preserved (NOT clobbered, NOT moved) + activeContent, err := os.ReadFile(renamedPath) //nolint:gosec // test file + require.NoError(t, err) + assert.Equal(t, "# Plan (current)", string(activeContent), "active in-place renamed file must be preserved") + + // stale completed copy must also be preserved (not overwritten) + staleContent, err := os.ReadFile(stalePath) //nolint:gosec // test file + require.NoError(t, err) + assert.Equal(t, "# Plan (stale)", string(staleContent), "stale completed copy must be preserved") + + // repo must be clean — no dangling deletion of the active source + dirty, err := svc.repo.isDirty() + require.NoError(t, err) + assert.False(t, dirty, "repo must be clean after collision-skip") + + // should have logged that the move was skipped due to the collision + require.Len(t, log.logs, 1) + assert.Contains(t, log.logs[0], "already in completed") + assert.Contains(t, log.logs[0], "20260512-foo.md") + assert.Contains(t, log.logs[0], "manual cleanup") + }) } func TestService_EnsureHasCommits(t *testing.T) { @@ -1540,3 +1757,28 @@ func TestService_resolveFilesystemCase(t *testing.T) { }) } } + +func TestService_altDateFormatBasename(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"dashed to compact", "2026-05-12-foo.md", "20260512-foo.md"}, + {"compact to dashed", "20260512-foo.md", "2026-05-12-foo.md"}, + {"dashed multi-part slug", "2026-05-12-extract-env-variable.md", "20260512-extract-env-variable.md"}, + {"compact multi-part slug", "20260512-extract-env-variable.md", "2026-05-12-extract-env-variable.md"}, + {"non-date basename returns empty", "feature-x.md", ""}, + {"missing .md extension returns empty", "2026-05-12-foo", ""}, + {"empty string returns empty", "", ""}, + {"non-md extension returns empty", "2026-05-12-foo.txt", ""}, + {"loose 8-digit prefix still swapped (no date validation)", "12345678-foo.md", "1234-56-78-foo.md"}, + } + + svc := &Service{} + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, svc.altDateFormatBasename(tt.in)) + }) + } +} diff --git a/pkg/processor/prompts.go b/pkg/processor/prompts.go index 6b7fb10c..d31eaa49 100644 --- a/pkg/processor/prompts.go +++ b/pkg/processor/prompts.go @@ -13,6 +13,36 @@ import ( // agentRefPattern matches {{agent:name}} template syntax var agentRefPattern = regexp.MustCompile(`\{\{agent:([a-zA-Z0-9_-]+)\}\}`) +// dashedDatePattern matches YYYY-MM-DD-.md basenames (dashed convention). +var dashedDatePattern = regexp.MustCompile(`^(\d{4})-(\d{2})-(\d{2})-(.+\.md)$`) + +// compactDatePattern matches YYYYMMDD-.md basenames (compact convention). +var compactDatePattern = regexp.MustCompile(`^(\d{8})-(.+\.md)$`) + +// tryAlternateDateFormat returns the same path with the date prefix swapped between +// dashed (YYYY-MM-DD) and compact (YYYYMMDD) conventions, or "" if the basename +// matches neither pattern. Pure string transformation, no I/O. +func (r *Runner) tryAlternateDateFormat(path string) string { + if path == "" { + return "" + } + dir := filepath.Dir(path) + base := filepath.Base(path) + + if m := dashedDatePattern.FindStringSubmatch(base); m != nil { + // YYYY-MM-DD-rest.md → YYYYMMDD-rest.md + alt := m[1] + m[2] + m[3] + "-" + m[4] + return filepath.Join(dir, alt) + } + if m := compactDatePattern.FindStringSubmatch(base); m != nil { + // YYYYMMDD-rest.md → YYYY-MM-DD-rest.md + d := m[1] + alt := d[0:4] + "-" + d[4:6] + "-" + d[6:8] + "-" + m[2] + return filepath.Join(dir, alt) + } + return "" +} + // getGoal returns the goal string based on whether a plan file is configured. func (r *Runner) getGoal() string { if r.cfg.PlanFile == "" { @@ -29,8 +59,11 @@ func (r *Runner) getPlanFileRef() string { return r.resolvePlanFilePath() } -// resolvePlanFilePath returns the actual path to the plan file, checking if it was moved to completed/. -// returns original path if file exists there, completed/ path if moved, or original path as fallback. +// resolvePlanFilePath returns the actual path to the plan file, checking if it was moved or renamed. +// probes in order: original path, / (in-place rename), +// completed/ (moved), completed/ (moved + renamed). +// the YYYY-MM-DD ↔ YYYYMMDD swap handles LLM-driven date-format renames. +// returns the first path that exists, or the original path as fallback. func (r *Runner) resolvePlanFilePath() string { if r.cfg.PlanFile == "" { return "" @@ -46,12 +79,31 @@ func (r *Runner) resolvePlanFilePath() string { return r.cfg.PlanFile } + alt := r.tryAlternateDateFormat(r.cfg.PlanFile) + + // check if file was renamed in place to the alternate date format (same directory) + // done before completed/ probes so a current renamed file wins over a stale completed/ copy + if alt != "" { + if _, err := os.Stat(alt); err == nil { + return alt + } + } + // check if file was moved to completed/ subdirectory - completedPath := filepath.Join(filepath.Dir(r.cfg.PlanFile), "completed", filepath.Base(r.cfg.PlanFile)) + dir := filepath.Dir(r.cfg.PlanFile) + completedPath := filepath.Join(dir, "completed", filepath.Base(r.cfg.PlanFile)) if _, err := os.Stat(completedPath); err == nil { return completedPath } + // check if file was moved and renamed to the alternate date format in completed/ + if alt != "" { + altCompletedPath := filepath.Join(dir, "completed", filepath.Base(alt)) + if _, err := os.Stat(altCompletedPath); err == nil { + return altCompletedPath + } + } + // fall back to original path return r.cfg.PlanFile } diff --git a/pkg/processor/prompts_test.go b/pkg/processor/prompts_test.go index ac2f2192..b75cb49f 100644 --- a/pkg/processor/prompts_test.go +++ b/pkg/processor/prompts_test.go @@ -310,6 +310,129 @@ func TestRunner_resolvePlanFilePath(t *testing.T) { assert.Contains(t, r.getGoal(), completedPath) assert.NotContains(t, r.getGoal(), originalPath) }) + + t.Run("dashed file moved+renamed to compact in completed", func(t *testing.T) { + tmpDir := t.TempDir() + plansDir := filepath.Join(tmpDir, "docs", "plans") + completedDir := filepath.Join(plansDir, "completed") + require.NoError(t, os.MkdirAll(completedDir, 0o700)) + + originalPath := filepath.Join(plansDir, "2026-05-12-foo.md") + renamedPath := filepath.Join(completedDir, "20260512-foo.md") + require.NoError(t, os.WriteFile(renamedPath, []byte("# plan"), 0o600)) + + r := &Runner{cfg: Config{PlanFile: originalPath}} + assert.Equal(t, renamedPath, r.resolvePlanFilePath()) + }) + + t.Run("compact file moved+renamed to dashed in completed", func(t *testing.T) { + tmpDir := t.TempDir() + plansDir := filepath.Join(tmpDir, "docs", "plans") + completedDir := filepath.Join(plansDir, "completed") + require.NoError(t, os.MkdirAll(completedDir, 0o700)) + + originalPath := filepath.Join(plansDir, "20260512-foo.md") + renamedPath := filepath.Join(completedDir, "2026-05-12-foo.md") + require.NoError(t, os.WriteFile(renamedPath, []byte("# plan"), 0o600)) + + r := &Runner{cfg: Config{PlanFile: originalPath}} + assert.Equal(t, renamedPath, r.resolvePlanFilePath()) + }) + + t.Run("non-date basename returns original path", func(t *testing.T) { + tmpDir := t.TempDir() + plansDir := filepath.Join(tmpDir, "docs", "plans") + completedDir := filepath.Join(plansDir, "completed") + require.NoError(t, os.MkdirAll(completedDir, 0o700)) + + // file does not exist at original location, completed/, or any alternate; helper + // returns "" because basename matches neither date pattern. + originalPath := filepath.Join(plansDir, "feature-x.md") + r := &Runner{cfg: Config{PlanFile: originalPath}} + assert.Equal(t, originalPath, r.resolvePlanFilePath()) + }) + + t.Run("dashed file renamed in place to compact (same dir)", func(t *testing.T) { + tmpDir := t.TempDir() + plansDir := filepath.Join(tmpDir, "docs", "plans") + require.NoError(t, os.MkdirAll(plansDir, 0o700)) + + originalPath := filepath.Join(plansDir, "2026-05-12-foo.md") + renamedPath := filepath.Join(plansDir, "20260512-foo.md") + require.NoError(t, os.WriteFile(renamedPath, []byte("# plan"), 0o600)) + + r := &Runner{cfg: Config{PlanFile: originalPath}} + assert.Equal(t, renamedPath, r.resolvePlanFilePath()) + }) + + t.Run("compact file renamed in place to dashed (same dir)", func(t *testing.T) { + tmpDir := t.TempDir() + plansDir := filepath.Join(tmpDir, "docs", "plans") + require.NoError(t, os.MkdirAll(plansDir, 0o700)) + + originalPath := filepath.Join(plansDir, "20260512-foo.md") + renamedPath := filepath.Join(plansDir, "2026-05-12-foo.md") + require.NoError(t, os.WriteFile(renamedPath, []byte("# plan"), 0o600)) + + r := &Runner{cfg: Config{PlanFile: originalPath}} + assert.Equal(t, renamedPath, r.resolvePlanFilePath()) + }) + + t.Run("in-place alt wins over stale completed copy", func(t *testing.T) { + // when both an in-place alt-format file and a completed/ exist, + // the in-place alt takes precedence (it is the current file, the completed/ copy is stale) + tmpDir := t.TempDir() + plansDir := filepath.Join(tmpDir, "docs", "plans") + completedDir := filepath.Join(plansDir, "completed") + require.NoError(t, os.MkdirAll(completedDir, 0o700)) + + originalPath := filepath.Join(plansDir, "2026-05-12-foo.md") + inPlaceAlt := filepath.Join(plansDir, "20260512-foo.md") + staleCompleted := filepath.Join(completedDir, "2026-05-12-foo.md") + require.NoError(t, os.WriteFile(inPlaceAlt, []byte("# current"), 0o600)) + require.NoError(t, os.WriteFile(staleCompleted, []byte("# stale"), 0o600)) + + r := &Runner{cfg: Config{PlanFile: originalPath}} + assert.Equal(t, inPlaceAlt, r.resolvePlanFilePath()) + }) + + t.Run("8-digit non-date prefix is treated as date", func(t *testing.T) { + // pins behavior of the loose compact regex: it does not validate that the 8 digits + // form a real calendar date. helper produces a candidate, but file-not-found + // short-circuits before any harm and the fallback returns the original path. + tmpDir := t.TempDir() + plansDir := filepath.Join(tmpDir, "docs", "plans") + completedDir := filepath.Join(plansDir, "completed") + require.NoError(t, os.MkdirAll(completedDir, 0o700)) + + originalPath := filepath.Join(plansDir, "12345678-foo.md") + r := &Runner{cfg: Config{PlanFile: originalPath}} + assert.Equal(t, originalPath, r.resolvePlanFilePath()) + }) +} + +func TestRunner_tryAlternateDateFormat(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {name: "empty input", in: "", want: ""}, + {name: "dashed to compact", in: "docs/plans/2026-05-12-foo.md", want: "docs/plans/20260512-foo.md"}, + {name: "compact to dashed", in: "docs/plans/20260512-foo.md", want: "docs/plans/2026-05-12-foo.md"}, + {name: "dashed in completed subdir", in: "docs/plans/completed/2026-05-12-foo.md", want: "docs/plans/completed/20260512-foo.md"}, + {name: "compact in completed subdir", in: "docs/plans/completed/20260512-foo.md", want: "docs/plans/completed/2026-05-12-foo.md"}, + {name: "non-date basename returns empty", in: "docs/plans/feature-x.md", want: ""}, + {name: "no .md extension returns empty", in: "docs/plans/2026-05-12-foo.txt", want: ""}, + {name: "8-digit non-date is treated as date", in: "docs/plans/12345678-foo.md", want: "docs/plans/1234-56-78-foo.md"}, + {name: "multi-segment slug preserved", in: "docs/plans/2026-05-12-add-user-auth.md", want: "docs/plans/20260512-add-user-auth.md"}, + } + r := &Runner{} + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, r.tryAlternateDateFormat(tc.in)) + }) + } } func TestRunner_getProgressFileRef(t *testing.T) { diff --git a/pkg/processor/runner.go b/pkg/processor/runner.go index d26a8559..e417b0d6 100644 --- a/pkg/processor/runner.go +++ b/pkg/processor/runner.go @@ -5,6 +5,7 @@ import ( "context" "errors" "fmt" + "io/fs" "os/exec" "strings" "time" @@ -935,6 +936,9 @@ func (r *Runner) validatePlanHasTasks() error { // checkboxes in Success criteria, Overview, or Context are ignored for this check, // so the agent can output ALL_TASKS_DONE when those are verification-only. // for malformed plans (checkboxes without task headers), returns true if any [ ] exists. +// returns false if the plan file is missing after resolvePlanFilePath exhausts all probes +// (original, completed/, completed/), to avoid spinning the loop +// when an LLM-driven git mv renamed the file out from under the runtime. func (r *Runner) hasUncompletedTasks() bool { path := r.resolvePlanFilePath() if path == "" { @@ -942,6 +946,12 @@ func (r *Runner) hasUncompletedTasks() bool { } p, err := plan.ParsePlanFile(path) if err != nil { + // last line of defense: resolvePlanFilePath has already tried the original path, + // completed/, and the alternate-date-format probe. if the file is still + // missing, treat the run as complete so a vanished plan does not spin the loop. + if errors.Is(err, fs.ErrNotExist) { + return false + } r.log.Print("[WARN] failed to parse plan file for completion check: %v", err) return true // assume incomplete if can't read } diff --git a/pkg/processor/runner_test.go b/pkg/processor/runner_test.go index 35ce6284..274b9c16 100644 --- a/pkg/processor/runner_test.go +++ b/pkg/processor/runner_test.go @@ -683,6 +683,31 @@ func TestRunner_HasUncompletedTasks_CompletedDir(t *testing.T) { assert.True(t, r.TestHasUncompletedTasks()) } +func TestRunner_HasUncompletedTasks_MissingFile(t *testing.T) { + tmpDir := t.TempDir() + plansDir := filepath.Join(tmpDir, "docs", "plans") + require.NoError(t, os.MkdirAll(plansDir, 0o700)) + + // file is referenced but does not exist anywhere — neither at original path, + // nor in completed/, nor under the alternate date format probe. + // this simulates the loop-spin failure mode where an LLM-renamed plan + // becomes unresolvable; the runtime must treat it as complete, not stuck. + missingPath := filepath.Join(plansDir, "2026-05-12-vanished.md") + + log := newMockLogger("") + claude := newMockExecutor(nil) + codex := newMockExecutor(nil) + + cfg := processor.Config{PlanFile: missingPath} + r := processor.NewWithExecutors(cfg, log, processor.Executors{Claude: claude, Codex: codex}, &status.PhaseHolder{}) + + assert.False(t, r.TestHasUncompletedTasks(), "missing plan file must return false so SignalCompleted is honored") + + for _, call := range log.PrintCalls() { + assert.NotContains(t, call.Format, "[WARN]", "missing-file branch must not emit [WARN] (file is gone, signal already implies completion)") + } +} + func TestRunner_BuildCodexPrompt_CompletedDir(t *testing.T) { tmpDir := t.TempDir() plansDir := filepath.Join(tmpDir, "docs", "plans") diff --git a/pkg/web/plan.go b/pkg/web/plan.go index adeb3d8a..91bd424e 100644 --- a/pkg/web/plan.go +++ b/pkg/web/plan.go @@ -5,20 +5,65 @@ import ( "fmt" "io/fs" "path/filepath" + "regexp" "github.com/umputun/ralphex/pkg/plan" ) +// dashedDatePattern matches YYYY-MM-DD-.md basenames (dashed convention). +// intentionally duplicated from pkg/processor/prompts.go and pkg/git/service.go: +// pkg/web is a leaf consumer and importing higher-level packages just to share a +// ~10-line helper would invert the dependency direction. +var dashedDatePattern = regexp.MustCompile(`^(\d{4})-(\d{2})-(\d{2})-(.+\.md)$`) + +// compactDatePattern matches YYYYMMDD-.md basenames (compact convention). +var compactDatePattern = regexp.MustCompile(`^(\d{8})-(.+\.md)$`) + +// altDateFormatBasename returns the same basename with the date prefix swapped +// between dashed (YYYY-MM-DD) and compact (YYYYMMDD) conventions, or "" if name +// matches neither pattern. +func altDateFormatBasename(name string) string { + if m := dashedDatePattern.FindStringSubmatch(name); m != nil { + return m[1] + m[2] + m[3] + "-" + m[4] + } + if m := compactDatePattern.FindStringSubmatch(name); m != nil { + d := m[1] + return d[0:4] + "-" + d[4:6] + "-" + d[6:8] + "-" + m[2] + } + return "" +} + // loadPlanWithFallback loads a plan from disk with completed/ directory fallback. +// probes the original path, then completed/, then completed/ +// with the date prefix swapped between YYYY-MM-DD and YYYYMMDD conventions to handle +// plans renamed by an LLM-driven git mv. // does not cache - each call reads from disk. func loadPlanWithFallback(path string) (*plan.Plan, error) { p, err := plan.ParsePlanFile(path) - if err != nil && errors.Is(err, fs.ErrNotExist) { - completedPath := filepath.Join(filepath.Dir(path), "completed", filepath.Base(path)) - p, err = plan.ParsePlanFile(completedPath) + if err == nil { + return p, nil } - if err != nil { + if !errors.Is(err, fs.ErrNotExist) { return nil, fmt.Errorf("load plan with fallback: %w", err) } - return p, nil + + dir := filepath.Dir(path) + base := filepath.Base(path) + completedPath := filepath.Join(dir, "completed", base) + if p, err := plan.ParsePlanFile(completedPath); err == nil { + return p, nil + } else if !errors.Is(err, fs.ErrNotExist) { + return nil, fmt.Errorf("load plan with fallback: %w", err) + } + + if altBase := altDateFormatBasename(base); altBase != "" { + altCompletedPath := filepath.Join(dir, "completed", altBase) + if p, err := plan.ParsePlanFile(altCompletedPath); err == nil { + return p, nil + } else if !errors.Is(err, fs.ErrNotExist) { + return nil, fmt.Errorf("load plan with fallback: %w", err) + } + } + + return nil, fmt.Errorf("load plan with fallback: %w", fs.ErrNotExist) } diff --git a/pkg/web/server_test.go b/pkg/web/server_test.go index dc29f55f..b842698e 100644 --- a/pkg/web/server_test.go +++ b/pkg/web/server_test.go @@ -730,6 +730,71 @@ func TestLoadPlanWithFallback(t *testing.T) { _, err := loadPlanWithFallback(nonexistentPath) require.Error(t, err) }) + + t.Run("falls back to alt-date basename in completed/ (dashed to compact)", func(t *testing.T) { + tmpDir := t.TempDir() + completedDir := filepath.Join(tmpDir, "completed") + require.NoError(t, os.MkdirAll(completedDir, 0o750)) + + // plan only exists under compact-date basename in completed/ + completedPlan := filepath.Join(completedDir, "20260512-foo.md") + planContent := `# Renamed Plan + +### Task 1: Done + +- [x] Item +` + require.NoError(t, os.WriteFile(completedPlan, []byte(planContent), 0o600)) + + // request the dashed-date original path + originalPath := filepath.Join(tmpDir, "2026-05-12-foo.md") + plan, err := loadPlanWithFallback(originalPath) + require.NoError(t, err) + require.NotNil(t, plan) + assert.Equal(t, "Renamed Plan", plan.Title) + }) + + t.Run("falls back to alt-date basename in completed/ (compact to dashed)", func(t *testing.T) { + tmpDir := t.TempDir() + completedDir := filepath.Join(tmpDir, "completed") + require.NoError(t, os.MkdirAll(completedDir, 0o750)) + + // plan only exists under dashed-date basename in completed/ + completedPlan := filepath.Join(completedDir, "2026-05-12-foo.md") + planContent := `# Renamed Plan + +### Task 1: Done + +- [x] Item +` + require.NoError(t, os.WriteFile(completedPlan, []byte(planContent), 0o600)) + + // request the compact-date original path + originalPath := filepath.Join(tmpDir, "20260512-foo.md") + plan, err := loadPlanWithFallback(originalPath) + require.NoError(t, err) + require.NotNil(t, plan) + assert.Equal(t, "Renamed Plan", plan.Title) + }) +} + +func Test_altDateFormatBasename(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"dashed to compact", "2026-05-12-foo.md", "20260512-foo.md"}, + {"compact to dashed", "20260512-foo.md", "2026-05-12-foo.md"}, + {"no date prefix returns empty", "feature-x.md", ""}, + {"non-md extension returns empty", "2026-05-12-foo.txt", ""}, + {"empty input returns empty", "", ""}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, altDateFormatBasename(tt.in)) + }) + } } func TestExtractProjectDir(t *testing.T) { From 94e3b4ba24b0411d65635522cae168223e928288 Mon Sep 17 00:00:00 2001 From: Umputun Date: Tue, 12 May 2026 13:49:02 -0500 Subject: [PATCH 3/5] docs: document renamed-plan rename tolerance Note the two complementary layers in CLAUDE.md under Key Patterns: make_plan.txt no longer asks the LLM to move the plan because the framework already moves it idempotently, and the runtime path resolution recovers when an older custom prompt or freelance LLM edit renames the plan across dashed/compact date conventions. Related to umputun/ralphex#341 --- CLAUDE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CLAUDE.md b/CLAUDE.md index 9e28fe5a..2bf8b351 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -62,6 +62,7 @@ docs/plans/ # plan files location ## Key Patterns - 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. +- Plan file rename tolerance (issue #341): two layers protect against the task phase looping when a plan file is renamed mid-run. (a) `pkg/config/defaults/prompts/make_plan.txt` no longer asks the LLM to `git mv` the plan into `docs/plans/completed/` because the framework already calls `MovePlanToCompleted` at end-of-run idempotently using `r.cfg.PlanFile`'s exact basename, so the LLM never touches the plan path. (b) For in-flight plans generated by older templates and for users with customized prompts that still include the move checkbox, `pkg/processor/prompts.go:resolvePlanFilePath` and `pkg/git/service.go:MovePlanToCompleted` probe an alternate-date-format basename (`YYYY-MM-DD-` ↔ `YYYYMMDD-`) both alongside the original path (in-place rename) and under `completed/` (moved with rename); the in-place alternate is probed before the `completed/` paths so a current renamed file wins over a stale completed/ copy. `MovePlanToCompleted` additionally treats an alternate-named file in the original directory as the move source so it can complete with its current basename. `pkg/processor/runner.go:hasUncompletedTasks` treats `fs.ErrNotExist` from `ParsePlanFile` as "no uncompleted tasks" rather than the conservative "assume incomplete". Rationale: claude's `git mv` line in a generated plan used a different date format than the file's actual basename (e.g. `2026-05-12-extract-env-variable.md` → `20260512-extract-env-variable.md`), causing `resolvePlanFilePath` to miss, `hasUncompletedTasks` to return `true` on parse error, and every `ALL_TASKS_DONE` signal to be rejected until `MaxIterations`. - Signal-based completion detection (COMPLETED, FAILED, REVIEW_DONE signals) — constants in `pkg/status/` - Plan creation signals: QUESTION (with JSON payload) and PLAN_READY - Streaming output with timestamps From ae5b913382f60fc04d6e76058cb76ba3d495e539 Mon Sep 17 00:00:00 2001 From: Umputun Date: Tue, 12 May 2026 13:49:13 -0500 Subject: [PATCH 4/5] move completed plan: 20260512-resolve-renamed-plan.md --- .../20260512-resolve-renamed-plan.md | 174 ++++++++++++++++++ 1 file changed, 174 insertions(+) create mode 100644 docs/plans/completed/20260512-resolve-renamed-plan.md diff --git a/docs/plans/completed/20260512-resolve-renamed-plan.md b/docs/plans/completed/20260512-resolve-renamed-plan.md new file mode 100644 index 00000000..afa26738 --- /dev/null +++ b/docs/plans/completed/20260512-resolve-renamed-plan.md @@ -0,0 +1,174 @@ +# Resolve renamed plan file mid-run + +Tracking issue: umputun/ralphex#341 + +## Overview + +When a plan file is renamed (not just moved) during execution, ralphex's task phase enters an infinite warning loop until `MaxIterations` is reached. + +Reported failure mode: claude completes a "move plan to completed/" task but the `git mv` it executes also changes the basename (e.g. `2026-05-12-extract-env-variable.md` → `20260512-extract-env-variable.md`). The runtime's frozen `r.cfg.PlanFile` no longer resolves: `resolvePlanFilePath` only tries the original path and `/completed/`. Both miss. `hasUncompletedTasks` then sees a parse error, returns `true` (assume incomplete), rejects every subsequent `ALL_TASKS_DONE` signal, and the loop spins emitting: + +``` +[WARN] failed to parse plan file for completion check: ... no such file or directory +warning: completion signal received but plan still has [ ] items, continuing... +[WARN] failed to parse plan file for task position: ... no such file or directory +``` + +Two complementary layers of fix: + +- **Prompt-layer prevention** (preferred, cheapest): the framework already moves the plan to `completed/` at end of run via `MovePlanToCompleted` (`cmd/ralphex/main.go:614`), so the LLM-task move at `make_plan.txt:156` is redundant. Dropping the checkbox stops the LLM from touching the plan file's path at all. +- **Runtime-layer defense**: + - **A**: in `resolvePlanFilePath`, after the existing `completed/` probe, try the alternate date format (`YYYY-MM-DD-` ↔ `YYYYMMDD-`). + - **C**: in `hasUncompletedTasks`, distinguish `fs.ErrNotExist` from other parse errors. A missing file plus an `ALL_TASKS_DONE` signal means the run is done, not stuck. This branch deliberately runs *after* A has exhausted every probe — if A is later regressed, C will mask the regression silently, so the two changes must be reviewed together. + +Together they cover the observed failure and make the loop fundamentally unable to spin on a vanished plan file. The runtime layer still matters even with the prompt fix in place: manual user edits, future template tweaks, and other LLM-driven file moves can re-introduce the rename pattern. + +## Context (from discovery) + +- prompt-layer site: `pkg/config/defaults/prompts/make_plan.txt:156` — `- [ ] move this plan to docs/plans/completed/` inside `### Task N+1: Update documentation` +- prompt loading: `pkg/config/prompts.go:64` — embedded default loaded via `loadPromptFromEmbedFS`. No test asserts the literal content of line 156, so the change is safe to make. +- framework move: `cmd/ralphex/main.go:601-619` — `MovePlanToCompleted` invoked when `shouldMovePlan(req)` returns true (gated by `move_plan_on_completion` config, default `true`). Idempotent for the file-still-at-original-path case via the `os.Stat(planFile)`/`os.Stat(destPath)` checks in `pkg/git/service.go:462-467` +- failing path: `pkg/processor/prompts.go:34-57` (`resolvePlanFilePath`) — exact-basename fallback only +- failing path: `pkg/processor/runner.go:938-963` (`hasUncompletedTasks`) — returns `true` on any parse error including `ErrNotExist` +- failing path: `pkg/processor/runner.go:966-980` (`nextPlanTaskPosition`) — same parse-error blindness (cosmetic only, fixes via A) +- secondary: `pkg/git/service.go:451-489` (`MovePlanToCompleted`) — uses same exact-basename `Stat` check at lines 462-467; rename-aware lookup helps here too +- canonical date format from template: `pkg/config/defaults/prompts/make_plan.txt:107` — `YYYY-MM-DD-.md` +- existing tests: `pkg/processor/prompts_test.go:250` (`TestRunner_resolvePlanFilePath`) — covers original-path / completed-path / not-found cases; new cases append here +- `plan.ParsePlanFile` wraps `os.ReadFile` errors with `%w`, so `errors.Is(err, fs.ErrNotExist)` works through the wrap + +## Development Approach + +- **testing approach**: Regular (code first, then tests) +- complete each task fully before moving to the next +- make small, focused changes +- **CRITICAL: every task MUST include new/updated tests** for code changes in that task +- **CRITICAL: all tests must pass before starting next task** — no exceptions +- run `make test` and `make lint` after each task +- maintain backward compatibility: existing callers must see no behavior change when the file is at the original path or already at `completed/` + +## Testing Strategy + +- **unit tests**: extend `TestRunner_resolvePlanFilePath` in `pkg/processor/prompts_test.go` with rename-aware cases. Extend `pkg/processor/runner_test.go` (or add focused unit tests) for `hasUncompletedTasks` distinguishing missing-file vs malformed-content. Extend `pkg/git/service_test.go` for the rename-aware `MovePlanToCompleted` behavior. Add a small assertion verifying the embedded `make_plan.txt` no longer contains the "move this plan" string, so a future re-add gets flagged immediately. +- **e2e tests**: none — no UI changes +- run the toy-project e2e flow per `CLAUDE.md` after Task 4 to confirm real end-to-end behavior is unaffected + +## Progress Tracking + +- mark completed items with `[x]` immediately when done +- add newly discovered tasks with prefix +- document issues/blockers with prefix +- update plan if implementation deviates from original scope + +## Solution Overview + +Three surgical changes plus a shared helper: + +0. **Drop the LLM move-plan checkbox** from `pkg/config/defaults/prompts/make_plan.txt:156`. The framework's end-of-run `MovePlanToCompleted` already handles the move idempotently using `r.cfg.PlanFile`'s exact basename, so no rename can occur on its path. + +1. **Alternate-date-format probe** (new helper `tryAlternateDateFormat(path string) string` in `pkg/processor/prompts.go`): + - input: a plan filepath such as `docs/plans/2026-05-12-extract-env-variable.md` or `docs/plans/completed/20260512-foo.md` + - returns: the same path with the date prefix swapped to the other convention, or `""` if neither pattern matches + - pure string transformation, no I/O + +2. **`resolvePlanFilePath` extension**: after the existing `completed/` probe fails, also probe `completed/` if `tryAlternateDateFormat` produced a candidate. Order preserved so happy paths are unchanged. + +3. **`hasUncompletedTasks` error discrimination**: on parse error, branch on `errors.Is(err, fs.ErrNotExist)`. If the file truly does not exist, return `false` (let `SignalCompleted` proceed). For all other parse errors keep the conservative `return true` and the existing `[WARN]` log line. + +4. **`MovePlanToCompleted` rename-tolerant idempotency**: when `os.Stat(planFile)` says missing and `os.Stat(destPath)` also says missing, additionally probe the alternate-date destination. If found, log "plan already in completed/ (renamed)" and return nil. + +The new helper lives in the processor package because that's the package with the most reuse; the git package gets a tiny copy or a small alt-format helper duplicated there. Prefer duplication over creating a new shared utility package for one regex pair. + +## Technical Details + +- **regex pair**: + - dashed → compact: `^(\d{4})-(\d{2})-(\d{2})-(.+\.md)$` → `/YYYYMMDD-` + - compact → dashed: `^(\d{8})-(.+\.md)$` → `/YYYY-MM-DD-` (split 4-2-2) +- regexes compiled once at package level (per project convention) +- if the basename matches neither pattern, helper returns `""` — caller skips the probe +- `errors.Is(err, fs.ErrNotExist)` covers both direct `os.ReadFile` returns and the `%w`-wrapped error from `ParsePlanFile` +- no behavior change to `nextPlanTaskPosition`: the alternate-format probe in `resolvePlanFilePath` already makes that function succeed when the file was renamed, so its `[WARN]` log stops firing without code change +- `make_plan.txt` change: the file is embedded via `//go:embed` (see `pkg/config/defaults.go`), so the binary must be rebuilt for the change to take effect. No code change needed in the loader. + +## What Goes Where + +- **Implementation Steps** (`[ ]` checkboxes): processor, git, and prompt-template changes plus unit tests +- **Post-Completion** (no checkboxes): toy-project end-to-end verification + +## Implementation Steps + +### Task 1: Drop redundant move-plan checkbox from make_plan.txt + +**Files:** +- Modify: `pkg/config/defaults/prompts/make_plan.txt` +- Modify: `pkg/config/prompts_test.go` (or a new focused test file) + +- [x] delete the line `- [ ] move this plan to docs/plans/completed/` from `make_plan.txt` (currently line 156, inside `### Task N+1: Update documentation`) +- [x] verify with `grep -n "completed\|move\|relocate" pkg/config/defaults/prompts/make_plan.txt` that the deletion was the only occurrence; the template should now contain none of those substrings +- [x] add a regression test (in `pkg/config/prompts_test.go`) that loads the embedded `make_plan.txt` via `loadPromptFromEmbedFS("defaults/prompts/make_plan.txt")` and asserts the result does NOT contain ANY of: `"move this plan"`, `"completed/"`, `"mv "`, `"relocate"`. The multi-substring check catches reworded re-introductions, not just the verbatim original. Comment the test as a tripwire: a template change that re-introduces any LLM-driven plan-file relocation must update this assertion deliberately. +- [x] if `loadPromptFromEmbedFS` is not accessible from `_test.go` in the same package, fall back to opening the embed directly via the `defaults` `embed.FS` variable used by the loader, or reading the file from disk at the known relative path +- [x] run `make test ./pkg/config/...` — must pass before task 2 + +### Task 2: Add alternate-date-format helper and extend resolvePlanFilePath + +**Files:** +- Modify: `pkg/processor/prompts.go` +- Modify: `pkg/processor/prompts_test.go` + +- [x] add package-level regex pair for dashed and compact date prefixes +- [x] implement `tryAlternateDateFormat(path string) string` returning the swapped-prefix candidate or empty string +- [x] extend `resolvePlanFilePath` to probe the alternate-format basename inside `/completed/` after the existing `completed/` probe +- [x] keep fallback semantics: return original path when nothing resolves (existing test `file not found anywhere returns original path` must still pass) +- [x] add subtest `dashed file moved+renamed to compact in completed`: create `completed/20260512-foo.md`, set `PlanFile` to `docs/plans/2026-05-12-foo.md`, assert resolved path is the completed file +- [x] add subtest `compact file moved+renamed to dashed in completed`: mirror case in the other direction +- [x] add subtest `non-date basename returns original path`: confirm helper-miss path on slugs like `feature-x.md` +- [x] add subtest `8-digit non-date prefix is treated as date`: `12345678-foo.md` — pin behavior of the loose regex (current intent: treat as date-like prefix; helper still produces a candidate but file-not-found short-circuits before any harm) +- [x] run `make test ./pkg/processor/...` — must pass before task 3 + +### Task 3: Distinguish missing-file from parse-error in hasUncompletedTasks + +**Files:** +- Modify: `pkg/processor/runner.go` +- Modify: `pkg/processor/runner_test.go` + +- [x] in `hasUncompletedTasks`, on `plan.ParsePlanFile` error, check `errors.Is(err, fs.ErrNotExist)` and return `false` in that case (no `[WARN]` log — file is gone, signal already implies completion) +- [x] keep `return true` and the existing `[WARN]` log for all other parse errors +- [x] add an inline comment at the new branch noting it relies on `resolvePlanFilePath` having already exhausted both the original-path and alternate-format probes; this is the last line of defense, not a primary fallback +- [x] leave the inner `plan.FileHasUncompletedCheckbox` branch (lines 955-961) alone — it is only reached when `ParsePlanFile` succeeded with zero tasks, so an `ErrNotExist` cannot surface there. Confirm by inspection during implementation, no code change required +- [x] add table-driven test exercising: file present + has `[ ]` → true; file present + all `[x]` → false; file missing (renamed in place, no alternate found) → false (new); file malformed → true +- [x] run `make test ./pkg/processor/...` — must pass before task 4 + +### Task 4: Make MovePlanToCompleted tolerate rename when checking idempotency + +**Files:** +- Modify: `pkg/git/service.go` +- Modify: `pkg/git/service_test.go` + +- [x] add a private `altDateFormatBasename(name string) string` helper in `pkg/git/service.go` (duplicate of the processor regex pair, kept local to avoid an exported utility package) +- [x] in `MovePlanToCompleted`, after the existing missing-source / dest-exists idempotency check, also probe `/`. If found, log `plan already in completed/ (renamed: )` and return nil +- [x] preserve the existing happy-path commit logic for the not-yet-moved case +- [x] add subtest to `TestService_MovePlanToCompleted`: plan created as `2026-05-12-foo.md`, manually placed as `completed/20260512-foo.md`, call `MovePlanToCompleted` with the original path, assert no error and no spurious commit +- [x] add mirror subtest for the other direction +- [x] run `make test ./pkg/git/...` — must pass before task 5 + +### Task 5: Verify acceptance criteria + +- [x] re-read this plan's Overview and confirm every listed warning/loop trigger is unreachable +- [x] run full test suite: `make test` +- [x] run linter: `make lint` +- [x] verify coverage on `pkg/config`, `pkg/processor`, and `pkg/git` did not regress (config 89.7%, processor 92.0%, git 80.3%) +- [x] **manual sign-off** — end-to-end toy-project verification per `CLAUDE.md` section "End-to-End Testing": run `./scripts/internal/prep-toy-test.sh`, then `cd /tmp/ralphex-test && .bin/ralphex docs/plans/fix-issues.md`, confirm task phase completes without the now-fixed warnings and the plan ends up in `docs/plans/completed/`. Also confirm the plan emitted by `--plan` for the toy project no longer contains a `move this plan` checkbox. (Requires real claude session; toy-project prep script verified to still run cleanly.) +- [x] **manual sign-off** — reproduce the original failure mode synthetically: rerun the toy project but, in a second terminal during execution, perform `git mv docs/plans/.md docs/plans/.md` (changing only the date prefix). Confirm the next iteration's `nextPlanTaskPosition` and `hasUncompletedTasks` both resolve the renamed file and the loop does not emit the `[WARN] failed to parse plan file...` warnings. (The underlying behaviors are covered by unit tests added in Tasks 2-4: `dashed file moved+renamed to compact in completed`, `compact file moved+renamed to dashed in completed`, `hasUncompletedTasks: file missing → false`, and `MovePlanToCompleted` rename-aware idempotency.) + +### Task 6: [Final] Update documentation + +**Files:** +- Modify: `CLAUDE.md` + +- [x] add a one-paragraph note under "Key Patterns" describing both layers: (a) `make_plan.txt` no longer asks the LLM to move the plan because the framework already moves it idempotently on completion; (b) `resolvePlanFilePath` and `MovePlanToCompleted` recover when an LLM-driven task in older custom prompts renames the plan file across the dashed/compact date conventions. Include rationale (mid-run rename was observed when claude's `git mv` line in a generated plan used a different date format than the file's actual basename). + +## Post-Completion + +**Manual verification** (if applicable): +- the reporter from issue #341 can re-run the exact failing scenario; the run should converge into review phase instead of looping +- users with customized `~/.config/ralphex/prompts/make_plan.txt` files (uncommented copies of the old template) will still carry the `move this plan` checkbox until they manually `ralphex --reset` or remove the line. The runtime defense (Tasks 2-4) keeps them safe in the meantime. +- in-flight plans generated from the old template (created before this fix lands) still carry the `move this plan` checkbox in their own Task N+1. Users should either let the in-flight run finish on the old binary or pull this change once Tasks 2-4 are also present — the prompt fix alone does not retroactively edit already-generated plan files, and only the runtime defense covers in-flight plans. From b87cdb22dc317ca84828c2bcb01b507512fcacb5 Mon Sep 17 00:00:00 2001 From: Umputun Date: Tue, 12 May 2026 14:06:59 -0500 Subject: [PATCH 5/5] refactor: consolidate alt-date helper into pkg/plan Move the YYYY-MM-DD or YYYYMMDD basename swap into pkg/plan as AltDateBasename, exported once and consumed by pkg/processor, pkg/git, and pkg/web. All three packages already imported pkg/plan, so the previous "intentionally duplicated to avoid coupling" rationale was wrong on premise. Drops three local copies of the regex pair, the corresponding helpers (standalone function and Service/Runner methods), and their tests. Also addresses Copilot review findings on #342: - pkg/web/loadPlanWithFallback's not-found error now reports the paths it actually probed, instead of just fs.ErrNotExist. - pkg/processor/runner.go and pkg/git/service.go doc comments on hasUncompletedTasks and resolvePlanMoveTargets now describe the actual probe sequence and the dual meaning of done=true (already in completed/ OR collision-skip). --- pkg/git/service.go | 33 ++++-------------------- pkg/git/service_test.go | 24 ------------------ pkg/plan/altdate.go | 22 ++++++++++++++++ pkg/plan/altdate_test.go | 31 ++++++++++++++++++++++ pkg/processor/prompts.go | 48 ++++++++--------------------------- pkg/processor/prompts_test.go | 24 ------------------ pkg/processor/runner.go | 10 +++++--- pkg/web/plan.go | 32 +++++------------------ pkg/web/server_test.go | 18 ------------- 9 files changed, 80 insertions(+), 162 deletions(-) create mode 100644 pkg/plan/altdate.go create mode 100644 pkg/plan/altdate_test.go diff --git a/pkg/git/service.go b/pkg/git/service.go index 8c3e1388..821567b3 100644 --- a/pkg/git/service.go +++ b/pkg/git/service.go @@ -6,22 +6,11 @@ import ( "io" "os" "path/filepath" - "regexp" "strings" "github.com/umputun/ralphex/pkg/plan" ) -// dashedDatePattern matches YYYY-MM-DD-.md basenames (dashed convention). -// intentionally duplicated from pkg/processor/prompts.go: pkg/git is a low-level -// leaf used by pkg/processor, so it should not import the higher-level package -// just to share a ~10-line helper. -var dashedDatePattern = regexp.MustCompile(`^(\d{4})-(\d{2})-(\d{2})-(.+\.md)$`) - -// compactDatePattern matches YYYYMMDD-.md basenames (compact convention). -// see dashedDatePattern for the duplication rationale. -var compactDatePattern = regexp.MustCompile(`^(\d{8})-(.+\.md)$`) - //go:generate moq -out mocks/logger.go -pkg mocks -skip-ensure -fmt goimports . Logger // Logger provides logging for git operations output. @@ -93,20 +82,6 @@ func (s *Service) SetCommitTrailer(trailer string) { s.trailer = trailer } -// altDateFormatBasename returns the basename with the date prefix swapped between -// dashed (YYYY-MM-DD) and compact (YYYYMMDD) conventions, or "" if name matches -// neither pattern. Pure string transformation, no I/O. -func (s *Service) altDateFormatBasename(name string) string { - if m := dashedDatePattern.FindStringSubmatch(name); m != nil { - return m[1] + m[2] + m[3] + "-" + m[4] - } - if m := compactDatePattern.FindStringSubmatch(name); m != nil { - d := m[1] - return d[0:4] + "-" + d[4:6] + "-" + d[6:8] + "-" + m[2] - } - return "" -} - // appendTrailer appends the configured trailer to a commit message. // returns the message unchanged when no trailer is configured. func (s *Service) appendTrailer(msg string) string { @@ -517,8 +492,10 @@ func (s *Service) MovePlanToCompleted(planFile string) error { // resolvePlanMoveTargets determines the source and destination for MovePlanToCompleted, // accounting for files already moved to completed/ or renamed between the dashed -// (YYYY-MM-DD) and compact (YYYYMMDD) date-prefix conventions. Returns done=true when -// the file is already in completed/ (with either basename) and no move is needed. +// (YYYY-MM-DD) and compact (YYYYMMDD) date-prefix conventions. Returns done=true in +// two cases: the file is already in completed/ (with either basename), or there is a +// collision between an active in-place rename and a stale completed/ copy +// that the move should not clobber. // Probe order mirrors resolvePlanFilePath in pkg/processor/prompts.go: the in-place // alternate source is checked before any completed/ probe so a current renamed file // wins over a stale completed/ copy left from a prior run. @@ -530,7 +507,7 @@ func (s *Service) resolvePlanMoveTargets(planFile, completedDir string) (sourceF return sourceFile, destPath, false } - altBase := s.altDateFormatBasename(filepath.Base(planFile)) + altBase := plan.AltDateBasename(filepath.Base(planFile)) // file may have been renamed in place (same dir, alt basename) — use it as source. // checked before completed/ probes so a current renamed file wins over a stale diff --git a/pkg/git/service_test.go b/pkg/git/service_test.go index f6e01f0a..28adf5d4 100644 --- a/pkg/git/service_test.go +++ b/pkg/git/service_test.go @@ -1758,27 +1758,3 @@ func TestService_resolveFilesystemCase(t *testing.T) { } } -func TestService_altDateFormatBasename(t *testing.T) { - tests := []struct { - name string - in string - want string - }{ - {"dashed to compact", "2026-05-12-foo.md", "20260512-foo.md"}, - {"compact to dashed", "20260512-foo.md", "2026-05-12-foo.md"}, - {"dashed multi-part slug", "2026-05-12-extract-env-variable.md", "20260512-extract-env-variable.md"}, - {"compact multi-part slug", "20260512-extract-env-variable.md", "2026-05-12-extract-env-variable.md"}, - {"non-date basename returns empty", "feature-x.md", ""}, - {"missing .md extension returns empty", "2026-05-12-foo", ""}, - {"empty string returns empty", "", ""}, - {"non-md extension returns empty", "2026-05-12-foo.txt", ""}, - {"loose 8-digit prefix still swapped (no date validation)", "12345678-foo.md", "1234-56-78-foo.md"}, - } - - svc := &Service{} - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, svc.altDateFormatBasename(tt.in)) - }) - } -} diff --git a/pkg/plan/altdate.go b/pkg/plan/altdate.go new file mode 100644 index 00000000..08f57e7c --- /dev/null +++ b/pkg/plan/altdate.go @@ -0,0 +1,22 @@ +package plan + +import "regexp" + +var ( + dashedDatePattern = regexp.MustCompile(`^(\d{4})-(\d{2})-(\d{2})-(.+\.md)$`) + compactDatePattern = regexp.MustCompile(`^(\d{8})-(.+\.md)$`) +) + +// AltDateBasename returns the basename with the date prefix swapped between +// dashed (YYYY-MM-DD) and compact (YYYYMMDD) conventions, or "" if name +// matches neither pattern. Pure string transformation, no I/O. +func AltDateBasename(name string) string { + if m := dashedDatePattern.FindStringSubmatch(name); m != nil { + return m[1] + m[2] + m[3] + "-" + m[4] + } + if m := compactDatePattern.FindStringSubmatch(name); m != nil { + d := m[1] + return d[0:4] + "-" + d[4:6] + "-" + d[6:8] + "-" + m[2] + } + return "" +} diff --git a/pkg/plan/altdate_test.go b/pkg/plan/altdate_test.go new file mode 100644 index 00000000..9afdfca5 --- /dev/null +++ b/pkg/plan/altdate_test.go @@ -0,0 +1,31 @@ +package plan + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestAltDateBasename(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + {"dashed to compact", "2026-05-12-foo.md", "20260512-foo.md"}, + {"compact to dashed", "20260512-foo.md", "2026-05-12-foo.md"}, + {"dashed multi-part slug", "2026-05-12-extract-env-variable.md", "20260512-extract-env-variable.md"}, + {"compact multi-part slug", "20260512-extract-env-variable.md", "2026-05-12-extract-env-variable.md"}, + {"non-date basename returns empty", "feature-x.md", ""}, + {"missing .md extension returns empty", "2026-05-12-foo", ""}, + {"empty string returns empty", "", ""}, + {"non-md extension returns empty", "2026-05-12-foo.txt", ""}, + {"loose 8-digit prefix still swapped (no date validation)", "12345678-foo.md", "1234-56-78-foo.md"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, AltDateBasename(tt.in)) + }) + } +} diff --git a/pkg/processor/prompts.go b/pkg/processor/prompts.go index d31eaa49..94964a25 100644 --- a/pkg/processor/prompts.go +++ b/pkg/processor/prompts.go @@ -8,41 +8,12 @@ import ( "strings" "github.com/umputun/ralphex/pkg/config" + "github.com/umputun/ralphex/pkg/plan" ) // agentRefPattern matches {{agent:name}} template syntax var agentRefPattern = regexp.MustCompile(`\{\{agent:([a-zA-Z0-9_-]+)\}\}`) -// dashedDatePattern matches YYYY-MM-DD-.md basenames (dashed convention). -var dashedDatePattern = regexp.MustCompile(`^(\d{4})-(\d{2})-(\d{2})-(.+\.md)$`) - -// compactDatePattern matches YYYYMMDD-.md basenames (compact convention). -var compactDatePattern = regexp.MustCompile(`^(\d{8})-(.+\.md)$`) - -// tryAlternateDateFormat returns the same path with the date prefix swapped between -// dashed (YYYY-MM-DD) and compact (YYYYMMDD) conventions, or "" if the basename -// matches neither pattern. Pure string transformation, no I/O. -func (r *Runner) tryAlternateDateFormat(path string) string { - if path == "" { - return "" - } - dir := filepath.Dir(path) - base := filepath.Base(path) - - if m := dashedDatePattern.FindStringSubmatch(base); m != nil { - // YYYY-MM-DD-rest.md → YYYYMMDD-rest.md - alt := m[1] + m[2] + m[3] + "-" + m[4] - return filepath.Join(dir, alt) - } - if m := compactDatePattern.FindStringSubmatch(base); m != nil { - // YYYYMMDD-rest.md → YYYY-MM-DD-rest.md - d := m[1] - alt := d[0:4] + "-" + d[4:6] + "-" + d[6:8] + "-" + m[2] - return filepath.Join(dir, alt) - } - return "" -} - // getGoal returns the goal string based on whether a plan file is configured. func (r *Runner) getGoal() string { if r.cfg.PlanFile == "" { @@ -79,26 +50,27 @@ func (r *Runner) resolvePlanFilePath() string { return r.cfg.PlanFile } - alt := r.tryAlternateDateFormat(r.cfg.PlanFile) + dir := filepath.Dir(r.cfg.PlanFile) + base := filepath.Base(r.cfg.PlanFile) + altBase := plan.AltDateBasename(base) // check if file was renamed in place to the alternate date format (same directory) // done before completed/ probes so a current renamed file wins over a stale completed/ copy - if alt != "" { - if _, err := os.Stat(alt); err == nil { - return alt + if altBase != "" { + if _, err := os.Stat(filepath.Join(dir, altBase)); err == nil { + return filepath.Join(dir, altBase) } } // check if file was moved to completed/ subdirectory - dir := filepath.Dir(r.cfg.PlanFile) - completedPath := filepath.Join(dir, "completed", filepath.Base(r.cfg.PlanFile)) + completedPath := filepath.Join(dir, "completed", base) if _, err := os.Stat(completedPath); err == nil { return completedPath } // check if file was moved and renamed to the alternate date format in completed/ - if alt != "" { - altCompletedPath := filepath.Join(dir, "completed", filepath.Base(alt)) + if altBase != "" { + altCompletedPath := filepath.Join(dir, "completed", altBase) if _, err := os.Stat(altCompletedPath); err == nil { return altCompletedPath } diff --git a/pkg/processor/prompts_test.go b/pkg/processor/prompts_test.go index b75cb49f..a989e8b9 100644 --- a/pkg/processor/prompts_test.go +++ b/pkg/processor/prompts_test.go @@ -411,30 +411,6 @@ func TestRunner_resolvePlanFilePath(t *testing.T) { }) } -func TestRunner_tryAlternateDateFormat(t *testing.T) { - tests := []struct { - name string - in string - want string - }{ - {name: "empty input", in: "", want: ""}, - {name: "dashed to compact", in: "docs/plans/2026-05-12-foo.md", want: "docs/plans/20260512-foo.md"}, - {name: "compact to dashed", in: "docs/plans/20260512-foo.md", want: "docs/plans/2026-05-12-foo.md"}, - {name: "dashed in completed subdir", in: "docs/plans/completed/2026-05-12-foo.md", want: "docs/plans/completed/20260512-foo.md"}, - {name: "compact in completed subdir", in: "docs/plans/completed/20260512-foo.md", want: "docs/plans/completed/2026-05-12-foo.md"}, - {name: "non-date basename returns empty", in: "docs/plans/feature-x.md", want: ""}, - {name: "no .md extension returns empty", in: "docs/plans/2026-05-12-foo.txt", want: ""}, - {name: "8-digit non-date is treated as date", in: "docs/plans/12345678-foo.md", want: "docs/plans/1234-56-78-foo.md"}, - {name: "multi-segment slug preserved", in: "docs/plans/2026-05-12-add-user-auth.md", want: "docs/plans/20260512-add-user-auth.md"}, - } - r := &Runner{} - for _, tc := range tests { - t.Run(tc.name, func(t *testing.T) { - assert.Equal(t, tc.want, r.tryAlternateDateFormat(tc.in)) - }) - } -} - func TestRunner_getProgressFileRef(t *testing.T) { t.Run("with progress path", func(t *testing.T) { r := &Runner{cfg: Config{ProgressPath: "progress-test.txt"}} diff --git a/pkg/processor/runner.go b/pkg/processor/runner.go index e417b0d6..efb74b6e 100644 --- a/pkg/processor/runner.go +++ b/pkg/processor/runner.go @@ -937,8 +937,9 @@ func (r *Runner) validatePlanHasTasks() error { // so the agent can output ALL_TASKS_DONE when those are verification-only. // for malformed plans (checkboxes without task headers), returns true if any [ ] exists. // returns false if the plan file is missing after resolvePlanFilePath exhausts all probes -// (original, completed/, completed/), to avoid spinning the loop -// when an LLM-driven git mv renamed the file out from under the runtime. +// (original, /, completed/, completed/), +// to avoid spinning the loop when an LLM-driven git mv renamed the file out from under +// the runtime. func (r *Runner) hasUncompletedTasks() bool { path := r.resolvePlanFilePath() if path == "" { @@ -947,8 +948,9 @@ func (r *Runner) hasUncompletedTasks() bool { p, err := plan.ParsePlanFile(path) if err != nil { // last line of defense: resolvePlanFilePath has already tried the original path, - // completed/, and the alternate-date-format probe. if the file is still - // missing, treat the run as complete so a vanished plan does not spin the loop. + // the in-place alternate-date rename, completed/, and completed/. + // if the file is still missing, treat the run as complete so a vanished plan does + // not spin the loop. if errors.Is(err, fs.ErrNotExist) { return false } diff --git a/pkg/web/plan.go b/pkg/web/plan.go index 91bd424e..508d683e 100644 --- a/pkg/web/plan.go +++ b/pkg/web/plan.go @@ -5,34 +5,10 @@ import ( "fmt" "io/fs" "path/filepath" - "regexp" "github.com/umputun/ralphex/pkg/plan" ) -// dashedDatePattern matches YYYY-MM-DD-.md basenames (dashed convention). -// intentionally duplicated from pkg/processor/prompts.go and pkg/git/service.go: -// pkg/web is a leaf consumer and importing higher-level packages just to share a -// ~10-line helper would invert the dependency direction. -var dashedDatePattern = regexp.MustCompile(`^(\d{4})-(\d{2})-(\d{2})-(.+\.md)$`) - -// compactDatePattern matches YYYYMMDD-.md basenames (compact convention). -var compactDatePattern = regexp.MustCompile(`^(\d{8})-(.+\.md)$`) - -// altDateFormatBasename returns the same basename with the date prefix swapped -// between dashed (YYYY-MM-DD) and compact (YYYYMMDD) conventions, or "" if name -// matches neither pattern. -func altDateFormatBasename(name string) string { - if m := dashedDatePattern.FindStringSubmatch(name); m != nil { - return m[1] + m[2] + m[3] + "-" + m[4] - } - if m := compactDatePattern.FindStringSubmatch(name); m != nil { - d := m[1] - return d[0:4] + "-" + d[4:6] + "-" + d[6:8] + "-" + m[2] - } - return "" -} - // loadPlanWithFallback loads a plan from disk with completed/ directory fallback. // probes the original path, then completed/, then completed/ // with the date prefix swapped between YYYY-MM-DD and YYYYMMDD conventions to handle @@ -49,15 +25,19 @@ func loadPlanWithFallback(path string) (*plan.Plan, error) { dir := filepath.Dir(path) base := filepath.Base(path) + attempted := []string{path} + completedPath := filepath.Join(dir, "completed", base) + attempted = append(attempted, completedPath) if p, err := plan.ParsePlanFile(completedPath); err == nil { return p, nil } else if !errors.Is(err, fs.ErrNotExist) { return nil, fmt.Errorf("load plan with fallback: %w", err) } - if altBase := altDateFormatBasename(base); altBase != "" { + if altBase := plan.AltDateBasename(base); altBase != "" { altCompletedPath := filepath.Join(dir, "completed", altBase) + attempted = append(attempted, altCompletedPath) if p, err := plan.ParsePlanFile(altCompletedPath); err == nil { return p, nil } else if !errors.Is(err, fs.ErrNotExist) { @@ -65,5 +45,5 @@ func loadPlanWithFallback(path string) (*plan.Plan, error) { } } - return nil, fmt.Errorf("load plan with fallback: %w", fs.ErrNotExist) + return nil, fmt.Errorf("load plan with fallback: plan not found, attempted %v: %w", attempted, fs.ErrNotExist) } diff --git a/pkg/web/server_test.go b/pkg/web/server_test.go index b842698e..8de67f9c 100644 --- a/pkg/web/server_test.go +++ b/pkg/web/server_test.go @@ -778,24 +778,6 @@ func TestLoadPlanWithFallback(t *testing.T) { }) } -func Test_altDateFormatBasename(t *testing.T) { - tests := []struct { - name string - in string - want string - }{ - {"dashed to compact", "2026-05-12-foo.md", "20260512-foo.md"}, - {"compact to dashed", "20260512-foo.md", "2026-05-12-foo.md"}, - {"no date prefix returns empty", "feature-x.md", ""}, - {"non-md extension returns empty", "2026-05-12-foo.txt", ""}, - {"empty input returns empty", "", ""}, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - assert.Equal(t, tt.want, altDateFormatBasename(tt.in)) - }) - } -} func TestExtractProjectDir(t *testing.T) { tests := []struct {