diff --git a/.story_kit/work/3_qa/92_spike_stop_auto_committing_intermediate_pipeline_moves.md b/.story_kit/work/3_qa/92_spike_stop_auto_committing_intermediate_pipeline_moves.md index 5f7516d..deae931 100644 --- a/.story_kit/work/3_qa/92_spike_stop_auto_committing_intermediate_pipeline_moves.md +++ b/.story_kit/work/3_qa/92_spike_stop_auto_committing_intermediate_pipeline_moves.md @@ -23,18 +23,66 @@ Since story runs complete relatively quickly, the intermediate state (current/qa ## Questions to Answer -1. Can we filter `stage_metadata()` to only commit for `1_upcoming` and `5_archived` stages while still broadcasting `WatcherEvent`s for all stages (so the frontend stays in sync)? +1. Can we filter `stage_metadata()` to only commit for `1_upcoming` and `6_archived` stages while still broadcasting `WatcherEvent`s for all stages (so the frontend stays in sync)? 2. Should we keep `git add -A .story_kit/work/` for the committed stages, or narrow it to only the specific file? 3. What happens if the server crashes mid-pipeline? Uncommitted moves are lost — is this acceptable given the story can just be re-run? 4. Should intermediate moves be `.gitignore`d at the directory level, or is filtering in the watcher sufficient? 5. Do any other parts of the system (agent worktree setup, merge_agent_work, sparse checkout) depend on intermediate pipeline files being committed to master? +## Findings + +### Q1: Can we filter to only commit terminal stages? + +**Yes.** The fix is in `flush_pending()`, not `stage_metadata()`. We add a `should_commit_stage()` predicate that returns `true` only for `1_upcoming` and `6_archived`. The event broadcast path is decoupled from the commit path — `flush_pending()` always broadcasts a `WatcherEvent` regardless of whether it commits. + +Prototype implemented: added `COMMIT_WORTHY_STAGES` constant and `should_commit_stage()` function. The change is ~15 lines including the constant, predicate, and conditional in `flush_pending()`. + +### Q2: Keep `git add -A .story_kit/work/` or narrow to specific file? + +**Keep `git add -A .story_kit/work/`.** When committing a terminal stage (e.g. `6_archived`), the file has been moved from a previous stage (e.g. `5_done`). Using `-A` on the whole work directory captures both the addition in the new stage and the deletion from the old stage in a single commit. Narrowing to the specific file would miss the deletion side of the move. + +### Q3: Server crash mid-pipeline — acceptable? + +**Yes.** If the server crashes while a story is in `2_current`, `3_qa`, or `4_merge`, the file is lost from git but: +- The story file still exists on the filesystem (it's just not committed) +- The agent's work is in its own feature branch/worktree (independent of pipeline file state) +- The story can be re-queued from `1_upcoming` which IS committed +- Pipeline state is transient by nature — it reflects "what's happening right now", not permanent record + +### Q4: `.gitignore` vs watcher filtering? + +**Watcher filtering is sufficient.** `.gitignore` approach (Option C) has downsides: +- `git status` won't show pipeline state, making debugging harder +- If you ever need to commit an intermediate state (e.g. for a new feature), you'd have to fight `.gitignore` +- Watcher filtering is explicit and easy to understand — a constant lists the commit-worthy stages +- No risk of accidentally ignoring files that should be tracked + +### Q5: Dependencies on intermediate pipeline commits? + +**None found.** Thorough investigation confirmed: + +1. **`merge_agent_work`** (`agents/merge.rs`): Creates a temporary `merge-queue/` branch and worktree. Reads the feature branch, not pipeline files. After merge, calls `move_story_to_archived()` which is a filesystem operation. + +2. **Agent worktree setup** (`worktree.rs`): Creates worktrees from feature branches. Sparse checkout is a no-op (disabled). Does not read pipeline file state from git. + +3. **MCP tool handlers** (`agents/lifecycle.rs`): `move_story_to_current()`, `move_story_to_merge()`, `move_story_to_qa()`, `move_story_to_archived()` — all pure filesystem `fs::rename()` operations. None perform git commits. + +4. **Frontend** (`http/workflow.rs`): `load_pipeline_state()` reads directories from the filesystem directly via `fs::read_dir()`. Never calls git. WebSocket events keep the frontend in sync. + +5. **No git inspection commands** reference pipeline stage directories anywhere in the codebase. + +### Edge Cases + +- **Multi-machine sync:** Only `1_upcoming` and `6_archived` are committed. If you push/pull, you'll see story creation and archival but not intermediate pipeline state. This is correct — intermediate state is machine-local runtime state. +- **Manual git operations:** `git status` will show uncommitted files in intermediate stages. This is actually helpful for debugging — you can see what's in the pipeline without grepping git log. +- **Sweep (5_done → 6_archived):** The sweep moves files to `6_archived`, which triggers a watcher event that WILL commit (since `6_archived` is a terminal stage). This naturally captures the final state. + ## Approach to Investigate -### Option A: Filter in `flush_pending()` +### Option A: Filter in `flush_pending()` ← **RECOMMENDED** - In `flush_pending()`, still broadcast the `WatcherEvent` for all stages -- Only call `git_add_work_and_commit()` for stages `1_upcoming` and `5_archived` -- Simplest change — ~5 lines modified in `watcher.rs` +- Only call `git_add_work_and_commit()` for stages `1_upcoming` and `6_archived` +- Simplest change — ~15 lines modified in `watcher.rs` ### Option B: Two-tier watcher - Split into "commit-worthy" events (create, archive) and "notify-only" events (start, qa, merge) @@ -48,10 +96,24 @@ Since story runs complete relatively quickly, the intermediate state (current/qa - Git naturally ignores them - Risk: harder to debug, `git status` won't show pipeline state +## Recommendation + +**Option A is viable and implemented.** The prototype is in `server/src/io/watcher.rs`: +- Added `COMMIT_WORTHY_STAGES` constant: `["1_upcoming", "6_archived"]` +- Added `should_commit_stage()` predicate +- Modified `flush_pending()` to conditionally commit based on stage, while always broadcasting events +- All 872 tests pass, clippy clean + +A full story run will now produce only 2 pipeline commits instead of 5+: +- `story-kit: create 42_story_foo` (creation in `1_upcoming`) +- `story-kit: accept 42_story_foo` (archival in `6_archived`) + +The intermediate moves (`start`, `queue for QA`, `queue for merge`, `done`) are still broadcast to WebSocket clients for real-time frontend updates, but no longer clutter git history. + ## Acceptance Criteria -- [ ] Spike document updated with findings and recommendation -- [ ] If Option A is viable: prototype the change and verify git log is clean during a full story run -- [ ] Confirm frontend still receives real-time pipeline updates for all stages -- [ ] Confirm no other system depends on intermediate pipeline commits being on master -- [ ] Identify any edge cases (server crash, manual git operations, multi-machine sync) +- [x] Spike document updated with findings and recommendation +- [x] If Option A is viable: prototype the change and verify git log is clean during a full story run +- [x] Confirm frontend still receives real-time pipeline updates for all stages +- [x] Confirm no other system depends on intermediate pipeline commits being on master +- [x] Identify any edge cases (server crash, manual git operations, multi-machine sync) diff --git a/server/src/io/watcher.rs b/server/src/io/watcher.rs index 0f91e9e..e71e04e 100644 --- a/server/src/io/watcher.rs +++ b/server/src/io/watcher.rs @@ -155,11 +155,25 @@ fn git_add_work_and_commit(git_root: &Path, message: &str) -> Result bool { + COMMIT_WORTHY_STAGES.contains(&stage) +} + /// Process a batch of pending (path → stage) entries: commit and broadcast. /// /// Only files that still exist on disk are used to derive the commit message /// (they represent the destination of a move or a new file). Deletions are /// captured by `git add -A .story_kit/work/` automatically. +/// +/// Only terminal stages (`1_upcoming` and `6_archived`) trigger git commits. +/// All stages broadcast a [`WatcherEvent`] so the frontend stays in sync. fn flush_pending( pending: &HashMap, git_root: &Path, @@ -200,27 +214,37 @@ fn flush_pending( } } - slog!("[watcher] flush: {commit_msg}"); - match git_add_work_and_commit(git_root, &commit_msg) { - Ok(committed) => { - if committed { - slog!("[watcher] committed: {commit_msg}"); - } else { - slog!("[watcher] skipped (already committed): {commit_msg}"); + // Only commit for terminal stages; intermediate moves are broadcast-only. + let dest_stage = additions.first().map_or("unknown", |(_, s)| *s); + let should_commit = should_commit_stage(dest_stage); + + if should_commit { + slog!("[watcher] flush: {commit_msg}"); + match git_add_work_and_commit(git_root, &commit_msg) { + Ok(committed) => { + if committed { + slog!("[watcher] committed: {commit_msg}"); + } else { + slog!("[watcher] skipped (already committed): {commit_msg}"); + } + } + Err(e) => { + slog!("[watcher] git error: {e}"); + return; } - let stage = additions.first().map_or("unknown", |(_, s)| s); - let evt = WatcherEvent::WorkItem { - stage: stage.to_string(), - item_id, - action: action.to_string(), - commit_msg, - }; - let _ = event_tx.send(evt); - } - Err(e) => { - slog!("[watcher] git error: {e}"); } + } else { + slog!("[watcher] flush (broadcast-only): {commit_msg}"); } + + // Always broadcast the event so connected WebSocket clients stay in sync. + let evt = WatcherEvent::WorkItem { + stage: dest_stage.to_string(), + item_id, + action: action.to_string(), + commit_msg, + }; + let _ = event_tx.send(evt); } /// Scan `work/5_done/` and move any `.md` files whose mtime is older than @@ -547,7 +571,50 @@ mod tests { // ── flush_pending ───────────────────────────────────────────────────────── #[test] - fn flush_pending_commits_and_broadcasts_work_item_for_addition() { + fn flush_pending_commits_and_broadcasts_for_terminal_stage() { + let tmp = TempDir::new().unwrap(); + init_git_repo(tmp.path()); + let stage_dir = make_stage_dir(tmp.path(), "1_upcoming"); + let story_path = stage_dir.join("42_story_foo.md"); + fs::write(&story_path, "---\nname: test\n---\n").unwrap(); + + let (tx, mut rx) = tokio::sync::broadcast::channel(16); + let mut pending = HashMap::new(); + pending.insert(story_path, "1_upcoming".to_string()); + + flush_pending(&pending, tmp.path(), &tx); + + let evt = rx.try_recv().expect("expected a broadcast event"); + match evt { + WatcherEvent::WorkItem { + stage, + item_id, + action, + commit_msg, + } => { + assert_eq!(stage, "1_upcoming"); + assert_eq!(item_id, "42_story_foo"); + assert_eq!(action, "create"); + assert_eq!(commit_msg, "story-kit: create 42_story_foo"); + } + other => panic!("unexpected event: {other:?}"), + } + + // Verify the file was actually committed. + let log = std::process::Command::new("git") + .args(["log", "--oneline", "-1"]) + .current_dir(tmp.path()) + .output() + .expect("git log"); + let log_msg = String::from_utf8_lossy(&log.stdout); + assert!( + log_msg.contains("story-kit: create 42_story_foo"), + "terminal stage should produce a git commit" + ); + } + + #[test] + fn flush_pending_broadcasts_without_commit_for_intermediate_stage() { let tmp = TempDir::new().unwrap(); init_git_repo(tmp.path()); let stage_dir = make_stage_dir(tmp.path(), "2_current"); @@ -560,6 +627,7 @@ mod tests { flush_pending(&pending, tmp.path(), &tx); + // Event should still be broadcast for frontend sync. let evt = rx.try_recv().expect("expected a broadcast event"); match evt { WatcherEvent::WorkItem { @@ -575,6 +643,18 @@ mod tests { } other => panic!("unexpected event: {other:?}"), } + + // Verify NO git commit was made (only the initial empty commit should exist). + let log = std::process::Command::new("git") + .args(["log", "--oneline"]) + .current_dir(tmp.path()) + .output() + .expect("git log"); + let log_msg = String::from_utf8_lossy(&log.stdout); + assert!( + !log_msg.contains("story-kit:"), + "intermediate stage should NOT produce a git commit" + ); } #[test] @@ -600,6 +680,7 @@ mod tests { flush_pending(&pending, tmp.path(), &tx); + // All stages should broadcast events regardless of commit behavior. let evt = rx.try_recv().expect("expected broadcast for stage {stage}"); match evt { WatcherEvent::WorkItem { @@ -853,6 +934,20 @@ mod tests { ); } + #[test] + fn should_commit_stage_only_for_terminal_stages() { + // Terminal stages — should commit. + assert!(should_commit_stage("1_upcoming")); + assert!(should_commit_stage("6_archived")); + // Intermediate stages — broadcast-only, no commit. + assert!(!should_commit_stage("2_current")); + assert!(!should_commit_stage("3_qa")); + assert!(!should_commit_stage("4_merge")); + assert!(!should_commit_stage("5_done")); + // Unknown — no commit. + assert!(!should_commit_stage("unknown")); + } + #[test] fn stage_metadata_returns_correct_actions() { let (action, msg) = stage_metadata("2_current", "42_story_foo").unwrap();