Merge pull request #2 from yogeshkathayat/fix/ship-playbook-plan-reload-after-review

ship-playbook/SKILL.md

@@ -1,6 +1,6 @@
 ---
 name: ship-playbook
-version: 1.9.0
+version: 1.9.1
 description: |
   Take one feature request and run the entire delivery playbook automatically: plan it, review the
   plan, build it task by task, review the build, and optionally audit it for launch — then return the

ship-playbook/references/playbook-state.md

@@ -32,6 +32,10 @@ A intake ──> B plan ──> C plan-review loop (native ∥ harness, bounded)
   single reviewer (the FIX is always native). When it runs it's a plan-QUALITY gate (scope,
   decomposition, phantom paths): ONE bounded loop, exits on no BLOCK/CONCERN (OBSERVATIONs never block)
   OR non-convergence, capped at `MAX_REVIEW` (2).
+- **Fix-then-reload** — C's fix rounds edit the plan FILES on disk (`.json` source of truth, then re-render
+  `.md`), so the in-memory plan goes stale while the build (E) reads `plan.tasks` directly. After any review
+  that applied a fix, the run RELOADS the plan from disk before E so the build walks the FIXED DAG, not the
+  pre-fix one. If the reloaded plan fails validation, the pre-fix in-memory plan is kept.
 - E barriers between DAG layers; each task loops engineer↔reviewer until it passes — unless `taskReview
   skip`, then there is no per-task reviewer/fix loop and a task passes on its engineer validate alone.
   The per-task review is **slice-scoped**: it judges only the task's own writeScope + diff against its

ship-playbook/references/workflow-template.js

@@ -623,17 +623,19 @@ function ingestGoLiveGateMarkers(result) {
 // never block; only BLOCK/CONCERN count toward looping.)
 async function planReviewLoop(plan, who) {
   let prev = Infinity
+  let fixed = false                                    // did any fix round edit the plan files on disk?
   for (let i = 0; i < MAX_REVIEW; i++) {
     const r = await routeReview(who, founderBrief(plan, who), `plan-review:${who}:${i}`, 'Plan review')
-    if (!r) return { ran: false }                      // reviewer agent died → the plan gate did NOT run
-    if (r.gateNotRun) return { ran: false }            // configured reviewer (e.g. kiro CLI) could not run → NOT a clean/approved plan
+    if (!r) return { ran: false, fixed }               // reviewer agent died → the plan gate did NOT run
+    if (r.gateNotRun) return { ran: false, fixed }     // configured reviewer (e.g. kiro CLI) could not run → NOT a clean/approved plan
     const n = blockingCount(r.findings)
-    if (r.verdict === 'approve' || n === 0) return { ran: true }     // clean — OBSERVATIONs never block
-    if (n >= prev) return { ran: true }                // not improving → stop churning, proceed to build
+    if (r.verdict === 'approve' || n === 0) return { ran: true, fixed }     // clean — OBSERVATIONs never block
+    if (n >= prev) return { ran: true, fixed }         // not improving → stop churning, proceed to build
     prev = n
     await rAgent(planFixBrief(plan, r.findings), { label: `plan-fix:${i}`, phase: 'Plan review', schema: FIX_RESULT })   // fix is always native
+    fixed = true                                       // plan files on disk now differ from the in-memory `plan` → caller must reload before build
   }
-  return { ran: true }
+  return { ran: true, fixed }
 }
 
 // ── worktree hygiene: prune dangling build worktrees ────────────────────────────
@@ -1001,6 +1003,19 @@ if (!RESUME_PLAN && PLAN_REVIEW !== 'skip') {
   await statusStep({ status: 'plan_review', phases: { plan_review: { status: 'in_progress' } } }, 'plan-review-start')
   const pr = await planReviewLoop(plan, PLAN_REVIEW)
   planReviewRan = !!(pr && pr.ran)
+  // The fix loop edits the plan FILES on disk; the in-memory `plan` is now stale. Reload it so the BUILD
+  // walks the FIXED DAG, not the pre-fix one (the build reads plan.tasks directly — it never re-reads the
+  // files). Gated on pr.fixed so a clean review (no edits) skips the extra read. If the reloaded plan
+  // fails validation, keep the pre-fix in-memory plan rather than build on a plan we can't parse.
+  if (pr && pr.fixed) {
+    const reloadPath = plan.planPath || PLAN_PATH
+    if (reloadPath) {
+      const reloaded = await rAgent(loadPlanBrief(reloadPath), { label: 'reload-fixed-plan', phase: 'Plan review', schema: PLAN, agentType: 'general-purpose' })
+      const reErr = reloaded ? validatePlan(reloaded) : 'reload returned no plan'
+      if (reErr) log(`reloaded plan after fixes failed validation (${reErr}) — keeping the pre-fix in-memory plan`)
+      else { plan = reloaded; await statusStep({ plan: { name: plan.planName, path: plan.planPath || null, taskCount: plan.tasks.length }, tasks: Object.fromEntries(plan.tasks.map(t => [t.id, { title: t.title || '', agent: t.agent || '', branch: branchFor(t), status: priorStatus[t.id] || 'pending' }])) }, 'plan-reloaded') }
+    }
+  }
   await statusStep({ phases: { plan_review: { status: planReviewRan ? 'done' : 'errored' } } }, 'plan-review-done')
 } else {
   await statusStep({ phases: { plan_review: { status: 'skipped' } } }, 'plan-review-skip')