fix(state): all-skipped milestone routes to pre-planning, not validating
handleAllSlicesDone treated isStatusDone uniformly — "complete",
"done", AND "skipped" all counted as "milestone work is finished",
so a milestone whose only slice was skipped would advance to
phase=validating-milestone. That's wrong: a placeholder slice that
was skipped doesn't validate the milestone's success criteria, it
just clears the wedge.
Surfaced concretely in dr-repo M003 (Unified Dashboard + Pilot
Validation): I skipped the migration placeholder via the new
`sf headless skip-slice` CLI, and the next-dispatch reported
`validate-milestone M003` even though no real work had happened on
the milestone. The autonomous loop would then burn an LLM turn
running validate-milestone just to discover the obvious gap.
Fix: differentiate {complete, done} from {skipped} at the gate.
When zero slices carry real-work outcomes, route into the
pre-planning phase so the dispatcher's existing
discuss → research → plan ladder takes over. The PDD/vision is
already in the milestone row, so the planner has the purpose it
needs without operator hand-holding.
Verified end-to-end against dr-repo: `sf headless query` for M003
now reports phase=pre-planning and next dispatch
`roadmap-meeting M003` (the deep-planning entry rule fires first;
discuss/research/plan come after as artifacts land).
Tests: 4 cases — all-skipped → pre-planning, complete+skipped mix
→ validating, legacy "done" alias → validating, multiple skipped
→ pre-planning.
Resolves sf-mp73sk0m-63w88y (filed via headless feedback CLI).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
881fd5e304
commit
7dfb5099c9
2 changed files with 180 additions and 9 deletions
|
|
@ -6,7 +6,15 @@ import { existsSync, readdirSync } from "node:fs";
|
|||
import { join } from "node:path";
|
||||
import { loadFile, parseRequirementCounts, parseSummary } from "./files.js";
|
||||
import { findMilestoneIds } from "./milestone-ids.js";
|
||||
import { resolveMilestoneFile, resolveSfRootFile, resolveSliceFile, resolveSlicePath, resolveTaskFile, resolveTasksDir } from "./paths.js";
|
||||
import {
|
||||
resolveMilestoneFile,
|
||||
resolveSfRootFile,
|
||||
resolveSliceFile,
|
||||
resolveSlicePath,
|
||||
resolveTaskFile,
|
||||
resolveTasksDir,
|
||||
} from "./paths.js";
|
||||
import { loadQueueOrder, sortByQueueOrder } from "./queue-order.js";
|
||||
import {
|
||||
getAllMilestones,
|
||||
getMilestone,
|
||||
|
|
@ -18,9 +26,6 @@ import {
|
|||
getSliceTasks,
|
||||
isDbAvailable,
|
||||
} from "./sf-db.js";
|
||||
import { isClosedStatus, isDeferredStatus } from "./status-guards.js";
|
||||
import { loadQueueOrder, sortByQueueOrder } from "./queue-order.js";
|
||||
import { logWarning } from "./workflow-logger.js";
|
||||
import {
|
||||
canonicalMilestonePrefix,
|
||||
extractContextTitle,
|
||||
|
|
@ -30,6 +35,8 @@ import {
|
|||
readMilestoneValidationVerdict,
|
||||
stripMilestonePrefix,
|
||||
} from "./state-shared.js";
|
||||
import { isClosedStatus, isDeferredStatus } from "./status-guards.js";
|
||||
import { logWarning } from "./workflow-logger.js";
|
||||
|
||||
// ─── DB-backed State Derivation ────────────────────────────────────────────
|
||||
// isStatusDone replaced by isClosedStatus from status-guards.ts (single source of truth).
|
||||
|
|
@ -328,6 +335,18 @@ async function handleAllSlicesDone(
|
|||
)
|
||||
: true; // fall back to old behaviour if caller didn't pass slices
|
||||
if (!hasRealWork) {
|
||||
// Route into the pre-planning ladder. The dispatcher decides:
|
||||
//
|
||||
// no CONTEXT file -> discuss-milestone
|
||||
// has CONTEXT, no RESEARCH -> research-milestone
|
||||
// has both -> plan-milestone (which decomposes into real
|
||||
// slices alongside the existing skipped ones)
|
||||
//
|
||||
// The milestone's vision/PDD is already in the DB (set when
|
||||
// the milestone was created via `sf new-milestone`), so the
|
||||
// planner has the purpose it needs. No operator action
|
||||
// required - autonomous mode will pick it up on the next
|
||||
// dispatch tick.
|
||||
return {
|
||||
activeMilestone,
|
||||
activeSlice: null,
|
||||
|
|
@ -336,11 +355,13 @@ async function handleAllSlicesDone(
|
|||
recentDecisions: [],
|
||||
blockers: [],
|
||||
nextAction:
|
||||
`No slice on milestone ${activeMilestone.id} carries real work ` +
|
||||
`(every slice is skipped or none were ever planned). Run ` +
|
||||
`reassess-roadmap or close the milestone explicitly via ` +
|
||||
`\`sf headless complete-milestone ${activeMilestone.id}\` if the ` +
|
||||
`work is intentionally deferred.`,
|
||||
`Milestone ${activeMilestone.id} has no slice carrying real ` +
|
||||
`work (every slice is skipped). Route into the pre-planning ` +
|
||||
`ladder so the dispatcher picks discuss/research/plan based ` +
|
||||
`on which artifacts are missing. Use ` +
|
||||
`\`sf headless complete-milestone ${activeMilestone.id}\` ` +
|
||||
`only if you want to intentionally defer the work without ` +
|
||||
`planning it.`,
|
||||
registry,
|
||||
requirements,
|
||||
progress: { milestones: milestoneProgress, slices: sliceProgress },
|
||||
|
|
|
|||
|
|
@ -0,0 +1,150 @@
|
|||
/**
|
||||
* deriveState: when every slice on the active milestone was *skipped*
|
||||
* (not completed), route into the pre-planning ladder instead of the
|
||||
* validating-milestone phase.
|
||||
*
|
||||
* Background: the original handleAllSlicesDone branch treated
|
||||
* isStatusDone uniformly - "complete", "done", and "skipped" all
|
||||
* counted as "the milestone has finished its work". That collapsed
|
||||
* the migration-placeholder pattern (skip the stub slice -> milestone
|
||||
* advances to validation) with the genuinely-done case. The fix
|
||||
* differentiates them: zero slices in {complete, done} -> pre-planning
|
||||
* so the dispatcher's discuss -> research -> plan ladder takes over.
|
||||
*
|
||||
* This pins:
|
||||
* - all-skipped milestone routes to pre-planning, not validating
|
||||
* - at least one complete/done slice keeps the validating-milestone
|
||||
* behaviour (no regression for the happy path)
|
||||
* - the nextAction names the dispatcher path so an operator
|
||||
* reading the snapshot knows what'll happen autonomously
|
||||
*/
|
||||
|
||||
import assert from "node:assert/strict";
|
||||
import { mkdirSync, mkdtempSync, rmSync } from "node:fs";
|
||||
import { tmpdir } from "node:os";
|
||||
import { join } from "node:path";
|
||||
import { afterEach, test } from "vitest";
|
||||
import {
|
||||
closeDatabase,
|
||||
insertMilestone,
|
||||
insertSlice,
|
||||
isDbAvailable,
|
||||
openDatabase,
|
||||
} from "../sf-db.js";
|
||||
import { deriveState, invalidateStateCache } from "../state.js";
|
||||
|
||||
const tmpDirs = [];
|
||||
|
||||
afterEach(() => {
|
||||
closeDatabase();
|
||||
if (!isDbAvailable()) {
|
||||
openDatabase(":memory:");
|
||||
closeDatabase();
|
||||
}
|
||||
invalidateStateCache();
|
||||
while (tmpDirs.length > 0) {
|
||||
const dir = tmpDirs.pop();
|
||||
if (dir) rmSync(dir, { recursive: true, force: true });
|
||||
}
|
||||
});
|
||||
|
||||
function makeProject(milestoneId) {
|
||||
const dir = mkdtempSync(join(tmpdir(), "sf-state-allskipped-"));
|
||||
tmpDirs.push(dir);
|
||||
mkdirSync(join(dir, ".sf", "milestones", milestoneId, "slices", "S01"), {
|
||||
recursive: true,
|
||||
});
|
||||
openDatabase(join(dir, ".sf", "sf.db"));
|
||||
insertMilestone({
|
||||
id: milestoneId,
|
||||
title: "All-skipped milestone",
|
||||
status: "active",
|
||||
});
|
||||
return dir;
|
||||
}
|
||||
|
||||
test("all_slices_skipped_when_deriving_state_returns_pre_planning", async () => {
|
||||
const dir = makeProject("M501");
|
||||
insertSlice({
|
||||
milestoneId: "M501",
|
||||
id: "S01",
|
||||
title: "Migration placeholder",
|
||||
status: "skipped",
|
||||
sequence: 1,
|
||||
});
|
||||
|
||||
const state = await deriveState(dir);
|
||||
|
||||
assert.equal(state.activeMilestone?.id, "M501");
|
||||
assert.equal(state.phase, "pre-planning");
|
||||
// activeSlice is intentionally null - pre-planning needs the
|
||||
// dispatcher to pick the next unit type from artifacts.
|
||||
assert.equal(state.activeSlice, null);
|
||||
// nextAction must surface the autonomous path so operators know
|
||||
// SF will plan on its own.
|
||||
assert.match(
|
||||
state.nextAction,
|
||||
/pre-planning ladder|discuss\/research\/plan|every slice is skipped/i,
|
||||
);
|
||||
});
|
||||
|
||||
test("complete_slice_when_deriving_state_returns_validating_milestone", async () => {
|
||||
const dir = makeProject("M502");
|
||||
insertSlice({
|
||||
milestoneId: "M502",
|
||||
id: "S01",
|
||||
title: "Real work",
|
||||
status: "complete",
|
||||
sequence: 1,
|
||||
});
|
||||
insertSlice({
|
||||
milestoneId: "M502",
|
||||
id: "S02",
|
||||
title: "Migration placeholder",
|
||||
status: "skipped",
|
||||
sequence: 2,
|
||||
});
|
||||
|
||||
const state = await deriveState(dir);
|
||||
|
||||
assert.equal(state.activeMilestone?.id, "M502");
|
||||
assert.equal(state.phase, "validating-milestone");
|
||||
});
|
||||
|
||||
test("done_slice_when_deriving_state_returns_validating_milestone", async () => {
|
||||
const dir = makeProject("M503");
|
||||
insertSlice({
|
||||
milestoneId: "M503",
|
||||
id: "S01",
|
||||
title: "Legacy-style done slice",
|
||||
status: "done",
|
||||
sequence: 1,
|
||||
});
|
||||
|
||||
const state = await deriveState(dir);
|
||||
|
||||
assert.equal(state.activeMilestone?.id, "M503");
|
||||
assert.equal(state.phase, "validating-milestone");
|
||||
});
|
||||
|
||||
test("multiple_skipped_slices_when_deriving_state_returns_pre_planning", async () => {
|
||||
const dir = makeProject("M504");
|
||||
insertSlice({
|
||||
milestoneId: "M504",
|
||||
id: "S01",
|
||||
title: "First placeholder",
|
||||
status: "skipped",
|
||||
sequence: 1,
|
||||
});
|
||||
insertSlice({
|
||||
milestoneId: "M504",
|
||||
id: "S02",
|
||||
title: "Second placeholder",
|
||||
status: "skipped",
|
||||
sequence: 2,
|
||||
});
|
||||
|
||||
const state = await deriveState(dir);
|
||||
|
||||
assert.equal(state.phase, "pre-planning");
|
||||
});
|
||||
Loading…
Add table
Reference in a new issue