feat: add move_plan_on_completion config option#307
Conversation
1079694 to
ed80122
Compare
Plans part 1 of 2 for issue umputun#306: a boolean config option (default true) to suppress the move-to-completed step. Enables workflows that manage plan lifecycle externally (e.g. spec-driven tooling with separate archive steps) without coupling ralphex to any specific tool.
Introduces MovePlanOnCompletion and MovePlanOnCompletionSet fields on the Values struct, mirroring the FinalizeEnabled pattern. Adds an INI loader block for move_plan_on_completion and a merge block so local config cleanly overrides global even when the explicit value is false. Part of issue umputun#306, task 1 of the move_plan_on_completion config plan.
Consolidates the conditions for moving a completed plan file into a single testable helper. Wires MovePlanOnCompletion into the guard so users who set move_plan_on_completion=false in config keep the plan file in place.
- guard shouldMovePlan against nil req.Config to prevent latent nil-deref panic in the success path of executePlan; add nil_config test case - extend TestLoad_AllUserValues to cover move_plan_on_completion so the 'all user values' assertion actually includes the new field
ed80122 to
10456b7
Compare
umputun
left a comment
There was a problem hiding this comment.
few things worth fixing:
-
shouldMovePlannil-check exists only for tests (cmd/ralphex/main.go:828-830). No production caller passes a nilConfig. EveryexecutePlanRequestliteral assigns it, andexecutePlanalready dereferencesreq.Configat line 540 without a nil check. Thereq.Config != nilclause is there only soTestShouldMovePlan/nil_configpasses. Drop the nil branch and the test case. -
MovePlanOnCompletionSetonConfigis dead state (pkg/config/config.go:77). Production never reads it; onlyconfig_test.go:387,432assert it. The*Setfield is needed onValuesfor merge logic, but onConfig(post-assembly) it's unused. Other existing fields do the same (CodexEnabledSet,FinalizeEnabledSet, etc.) but no reason to perpetuate it for the new one. Drop the field onConfigand the matching assertions. -
default-true split across layers (
pkg/config/config.go:281-285). Default is applied post-assembly inloadConfigFromDirsinstead of in the embedded template. MirrorsNotifyOnError/NotifyOnCompleteprecedent so not strictly wrong, but cleaner to uncommentmove_plan_on_completion = trueinpkg/config/defaults/configand drop the post-assembly patch. Minor. -
redundant test assertions (
pkg/config/config_test.go:412,431-432). DedicatedTestLoad_MovePlanOnCompletionalready covers default/explicit-true/explicit-false;TestLoad_AllUserValuesdoesn't need to re-assert the same field. Resolves automatically if you apply #2.
naming: move_plan_on_completion doesn't quite match the project's _enabled / use_ convention (codex_enabled, finalize_enabled, use_worktree). I agreed to it in #306 so it's settled, just noting.
- drop nil-Config branch in shouldMovePlan (no production caller passes nil; was only there to satisfy a test case) - remove MovePlanOnCompletionSet from Config — it's post-assembly state that nothing in production reads; *Set is still tracked on Values for merge logic where it's actually needed - uncomment move_plan_on_completion = true in the embedded config template so the default flows through normal loader merge instead of being patched on in loadConfigFromDirs - drop redundant assertions from TestLoad_AllUserValues (the field already has dedicated coverage in TestLoad_MovePlanOnCompletion)
Summary
Adds
move_plan_on_completionconfig option (defaulttrue) that controls whether completed plans move todocs/plans/completed/on success. Set tofalsefor workflows that manage plan lifecycle externally (e.g. spec-driven tooling with separate archive steps).Part 1 of 2 for #306 (OpenSpec integration). Kept scope tight: no OpenSpec-specific detection here, just a generic toggle that any external workflow can use.
Changes
pkg/config/values.go—MovePlanOnCompletion+MovePlanOnCompletionSetfields, INI loader, merge block (local overrides global even when explicitfalse)pkg/config/config.go—MovePlanOnCompletiononConfigstruct, defaults totruepkg/config/defaults/config— commented template entrycmd/ralphex/main.go—shouldMovePlan()predicate guards the existingMovePlanToCompletedcall; nil-safe againstreq.Configvalues_test.go,config_test.go,main_test.go(including nil_config case)README.md,CLAUDE.md,llms.txtDefault behavior is unchanged.
Test plan
make testpassesmake lintcleantrue)move_plan_on_completion = falsein local config takes precedence over globaltrue