Fix T-820: Batch Remove Shifts Phase Boundaries

[ArjenSchwarz]

Summary

Batch remove operations on phased files were stripping all phase headers because cmd/batch.go only used phase-aware execution when an operation explicitly referenced a phase (op.Phase or add-phase). Plain removes took the non-phase path, destroying the phase structure.

Root Cause

The dispatch logic in executeBatchCommand checked hasPhaseOps (whether any operation names a phase), but not whether the file itself contains phases. The internal ExecuteBatchWithPhases already had the correct guard (len(phaseMarkers) > 0 || hasPhaseOps), but the CLI layer short-circuited before reaching it.

Fix

Always parse with ParseFileWithPhases so the file's own phase markers are detected. Use ExecuteBatchWithPhases when the file has phases or operations reference phases; fall back to regular ExecuteBatch only when neither condition applies.

Changes

• cmd/batch.go — Restructure dispatch to always detect file phases
• cmd/batch_test.go — Add regression test TestBatchCommand_RemoveOnPhasedFilePreservesPhases
• specs/bugfixes/batch-remove-shifts-phase-boundaries/report.md — Bugfix report

Testing

• Regression test confirms phase headers and task positions are preserved after batch remove
• Full test suite passes (make test)

[claude]

Code Review: Fix T-820 - Batch Remove Shifts Phase Boundaries

Overview

This PR fixes a real and impactful bug: batch remove operations on phased files were silently stripping all phase headers because the CLI dispatch logic only checked whether an operation referenced a phase, not whether the file itself contained phases. The fix is well-motivated, clearly explained, and comes with a regression test and a detailed bug report.

---

Strengths

• Minimal, targeted fix. The diff is concise - the offending conditional is replaced with a single unconditional parse, and the duplicate requirements-file setup block is naturally eliminated. Less code, same correctness.
• Aligns with existing internal guard. ExecuteBatchWithPhases already had the correct guard (len(phaseMarkers) > 0 || hasPhaseOps). The fix brings the CLI layer in sync rather than re-implementing the logic.
• Good error message cleanup. Simplifying the error string to just "loading task file" is correct now that the function is always called regardless of phase content.
• Regression test matches the bug exactly. The test exercises removes with no phase field on a file that has phase headers - precisely the scenario that was broken.
• Thorough bug report. The specs/bugfixes/ document records investigation dead-ends (the adjustPhaseMarkersForRemoval math hypothesis), useful for future readers.

---

Issues and Suggestions

Minor: Unchecked errors in the test

The test discards errors from os.Getwd(), os.Chdir(), and json.Marshal(). An os.Chdir failure in the deferred restore would leave the test suite in a wrong directory, potentially breaking subsequent tests. The active chdir should fail the test explicitly with t.Fatalf rather than silently proceeding.

Minor: Test verifies phase containment but not task renumbering after remove

The test uses strings.Index to verify phase header preservation and relative ordering. This covers the regression, but does not assert the new task IDs after two removes. If a future change breaks ID renumbering for phased files, this test would not catch it. Adding assertions on expected re-numbered IDs would make the test more robust - though this is a nice-to-have rather than a blocker.

Negligible: Performance overhead for non-phased files

ParseFileWithPhases now always runs instead of the cheaper ParseFile for non-phased files. For typical file sizes this is inconsequential. The parse-once approach is the correct default; an optimization can be deferred until measurable.

Observation: Fallback path coverage

There is no new test explicitly asserting that non-phased files still exercise the ExecuteBatch path after the refactor. Existing tests likely cover this implicitly; worth confirming before merge.

---

Summary

Category	Assessment
Correctness	Correct fix; root cause well-identified
Code quality	Clean reduction of duplication; good comments
Tests	Regression test accurate; minor error-handling gaps
Performance	Negligible overhead; acceptable trade-off
Security	No concerns

Solid, well-documented bugfix. The main item worth addressing before merge is the unchecked os.Chdir calls in the test, which could cause test suite pollution if they silently fail. The other points are non-blocking suggestions.

[ArjenSchwarz]

PR Review Overview - Iteration 1

PR: #90 | Branch: T-820/bugfix-batch-remove-shifts-phase-boundaries | Date: 2025-07-22

Valid Issues (all minor — no fixes required)

No blockers, critical, or major issues found.

Minor Issues (not fixed — non-blocking)

1. Unchecked errors in test — os.Getwd(), os.Chdir(), and json.Marshal() errors are discarded in the test. A silent os.Chdir failure in the deferred restore could pollute subsequent tests. Reviewer: @claude
2. Test does not assert task renumbering after remove — Phase header preservation and ordering are verified, but new task IDs after removes are not asserted. Nice-to-have for robustness. Reviewer: @claude

Negligible / Observations (no action needed)

3. Performance overhead for non-phased files — ParseFileWithPhases always runs now. Inconsequential for typical file sizes. Reviewer: @claude
4. Fallback path coverage — No new test for non-phased files exercising ExecuteBatch. Existing tests likely cover this implicitly. Reviewer: @claude

CI Status

All 12 checks passed ✅ — no CI issues to fix.

[claude]

Claude finished @ArjenSchwarz's task in 5m 40s —— View job

---

PR Review - Iteration 2

Addressed both minor issues from Iteration 1. All changes are in cmd/batch_test.go.

• Gather context and read the test file
• Fix unchecked errors in test (os.Getwd(), os.Chdir(), json.Marshal())
• Add task renumbering assertions after remove
• Commit and push changes

---

Changes made

cmd/batch_test.go

1. Fix unchecked errors — os.Getwd, os.Chdir, and json.Marshal now all use t.Fatalf on error. The deferred directory restore is replaced with t.Cleanup (Go testing best practice), which catches a silent os.Chdir failure that would otherwise leave subsequent tests in the wrong working directory.

2. Add task renumbering assertions — After removing tasks 1 and 3 from the phased file, the four remaining tasks must be renumbered 1–4. The batch engine applies removes highest-ID-first (3 then 1), so the expected result is:

1. Create design   (was 2)
2. Write tests     (was 4)
3. Deploy to staging  (was 5)
4. Deploy to production (was 6)

Also added explicit assertions that the removed tasks (Define requirements, Write code) no longer appear in the file.

3. Bonus fix: resetBatchFlags now clears dryRun — The existing resetBatchFlags helper only reset batchInput. TestBatchCommand_PositionalFileArg passes --dry-run as a CLI flag, which sets the global dryRun = true and leaves it set for subsequent tests. This caused the new test to run as a dry run silently: the original phase-position assertions still passed vacuously (the unchanged file already satisfied them), but the new removal and renumbering assertions exposed the bug. Adding dryRun = false and resetting the flag's Changed bit to resetBatchFlags fixes the inter-test pollution.

All unit tests pass (make test).
|

[claude]

Code Review: Fix T-820 - Batch Remove Shifts Phase Boundaries

Overview

This PR fixes a logic error in cmd/batch.go where the dispatch condition checked only whether operations referenced a phase, missing the case where the file itself contains phase headers. The fix is correct, minimal, and well-explained.

---

Code Quality and Correctness

cmd/batch.go

• Fix is sound. Always calling ParseFileWithPhases and then dispatching on len(phaseMarkers) > 0 || hasPhaseOps mirrors the guard already inside ExecuteBatchWithPhases. The CLI layer now delegates the decision to the same condition rather than duplicating it incorrectly.
• Nice DRY cleanup. The requirements-file setup was previously duplicated in both branches; consolidating it before the dispatch is a good improvement.
• Error message is correct. Changing "loading task file with phases" to "loading task file" is appropriate since this path now handles all files.
• Minor: comment could be tighter. The comment on the ParseFileWithPhases call says "ExecuteBatchWithPhases falls back to regular execution...", but the code does not actually rely on that — it explicitly dispatches to ExecuteBatch in the else-branch. Consider rewording to: "Always parse with phases so the file's own markers are detected; dispatch below picks the right executor."
• Non-phased file behavior is preserved. For files with no phases, ParseFileWithPhases returns an empty markers slice and hasPhaseOps stays false, so the existing ExecuteBatch path is still taken. The overhead versus ParseFile is negligible.

---

Test Coverage

TestBatchCommand_RemoveOnPhasedFilePreservesPhases

The test covers the happy path well, but has a few gaps worth addressing:

1. Missing: verify removed tasks are actually gone. The test confirms phase headers survive, but does not assert that tasks "Define requirements" and "Write code" were removed. A regression that preserves headers but skips the removal would pass today. Suggest adding:

if strings.Contains(contentStr, "Define requirements") {
    t.Error("Task 1 should have been removed")
}
if strings.Contains(contentStr, "Write code") {
    t.Error("Task 3 should have been removed")
}

2. Ignored errors in test setup. json.Marshal error is silently discarded (jsonData, _). Use t.Fatalf to avoid a confusing nil-bytes input if marshalling ever fails. The os.Chdir in the defer also ignores its error.

3. os.Chdir in tests. Safe here because the test does not call t.Parallel(), but any future parallelisation of the batch test suite would cause flaky failures. t.Chdir (Go 1.24+) automatically restores the directory and is the safer pattern going forward.

---

Security

No concerns. The change is purely dispatch logic with no new file path handling, no user-controlled format strings, and no new external inputs.

---

Performance

ParseFileWithPhases replaces ParseFile on all calls. The extra work is a single linear pass over content already in memory — negligible in practice.

---

Summary

The root cause analysis is correct, the fix is minimal and well-scoped, and the regression test covers the primary scenario. Main suggestions:

1. Add assertions that removed tasks are actually gone — strengthens the regression test meaningfully.
2. Use t.Chdir instead of os.Chdir for safer test isolation.
3. Tighten the comment on the ParseFileWithPhases call to avoid implying behaviour the code does not rely on.

These are minor — the fix itself is solid. Good work tracking down the root cause in the CLI dispatch layer rather than in the internal library.