plan engine: the start phase (epic #14081, Lot 1)

[ndeloof]

Lot 1 of #14081, built as three commits — each one CI-validated before the next lands on the branch (the PR stays draft until all three are green):

1. feat: reconciler plans the start phase (this push) — the plan vocabulary: OpWaitCondition (deduplicated per awaited service+condition, best-effort when every dependent is optional), OpRunPreStart (plan-time gating: only when no replica was running at observation), OpRunPostStart, PlanNode.Phase, replica chains preserving today's sequential start order, and ReconcileOptions.Scope (zero value = today's create-only plans, byte-identical — pinned by the existing 42 golden tests). Golden tests only, no consumer.
2. feat: executor runs start-phase operations — to follow once 1 is green.
3. refactor: split create() into preparePlan + execute — to follow once 2 is green.

🤖 Generated with Claude Code

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

[codecov]

Codecov Report

❌ Patch coverage is 76.44628% with 57 lines in your changes missing coverage. Please review.

Files with missing lines	Patch %	Lines
pkg/compose/executor_ops.go	49.01%	21 Missing and 5 partials ⚠️
pkg/compose/reconcile.go	86.70%	12 Missing and 9 partials ⚠️
pkg/compose/executor_events.go	66.66%	6 Missing ⚠️
pkg/compose/executor.go	42.85%	4 Missing ⚠️

📢 Thoughts on this report? Let us know!

[docker-agent]

Assessment: 🔴 CRITICAL

One high-severity bug was found in the start-phase planning code introduced by this PR.

[docker-agent]

Assessment: 🟢 APPROVE

The plan engine start phase implementation looks solid. The DAG-based approach to OpWaitCondition, OpRunPreStart, OpRunPostStart, and the replica chain ordering is well-structured. The zero-value ReconcileOptions.Scope preserves byte-identical behavior for existing create-only plans (confirmed by the 42 golden tests). No high or medium confidence bugs found in the changed code.

Lower-confidence findings (not posted inline)

• [medium] pkg/compose/reconcile.go:1119 — pre_start node placed in replica-1's start group causes Starting event to fire during pre_start hook, not during actual container start (confidence: weak 52/100)

  The preStart node is assigned the same group name as replica-1's container start node (fmt.Sprintf("start%s", strings.TrimPrefix(first.resID, "service"))). In buildGroupTracker, onNodeStart fires Starting when the first node in a group starts (gs.started == 1). With pre_start in the same group as start and post_start, the Starting progress event fires when the pre_start hook begins execution — potentially seconds before the container actually starts. The doneText = "Started" comment in the new code says this is intentional ("Started is emitted after the hooks ran — word-for-word the imperative sequence"), but the Starting signal fires earlier than users might expect. Worth a conscious decision on whether the group boundary should begin at the hook or at the container start.

[docker-agent]

Assessment: 🟢 APPROVE

The plan engine changes (OpWaitCondition, OpRunPreStart, OpRunPostStart, PlanNode.Phase, ReconcileOptions.Scope) are well-structured. The drafter identified one potential slice alias hazard in planServiceStart around the pre_start dep construction, but verification confirmed it is self-contained and harmless: the original prev slice is overwritten with a fresh single-element slice on line 1121 immediately after, so any backing-array mutation from the conditional append is never observable by subsequent code. The replica loop correctly uses slices.Clone(prev) on the updated slice. No real bugs introduced by this PR.

[docker-agent]

This review covers only the commits pushed since ba3c16642ed0fd78dba6dee06ce687523c02c857.

Assessment: 🟢 APPROVE

The refactor cleanly splits create() into preparePlan() and a thin caller. All error paths in preparePlan return nil, nil, nil, err consistently, the relative ordering of every step is preserved, and emitRunningEvents is correctly moved to the caller in create() — same position in the call sequence as before. No bugs introduced by this incremental change.

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

One medium-severity finding: a latent nil pointer dereference in newly introduced code. All other drafter hypotheses (slice aliasing in pre-start deps, missing OpRunPreStart/OpRunPostStart in event switch statements) were verified to be false positives — either the code flow is safe by construction or the cases are unreachable.

Lower-confidence findings (not posted inline)

(none — all below-threshold findings were dismissed)

[docker-agent]

Assessment: 🟡 NEEDS ATTENTION

This PR introduces the start-phase plan vocabulary and executor operations (OpWaitCondition, OpRunPreStart, OpRunPostStart) along with the ScopeCreateStart/ScopeStart reconcile scopes. The implementation is well-structured, with comprehensive golden tests confirming byte-identical output for the existing create-only scope. One medium-confidence issue was found in the new wait-condition error path.

[docker-agent]

[medium] execWaitCondition error path emits internal resource ID instead of user-friendly service name

When a required OpWaitCondition node fails, the error propagates to groups.onNodeError → emitErrorEvent, which uses op.ResourceID ("wait:db:service_healthy") as the progress-UI event ID. The best-effort skip path two lines above correctly uses "Service " + op.Name:

// best-effort skip: user sees "Service db: Skipped: no container to wait for"
exec.compose.events.On(skippedEvent("Service "+op.Name, "no container to wait for"))
return nil

// required fail: error propagates; emitErrorEvent fires with ID="wait:db:service_healthy"
return fmt.Errorf("required dependency %q has no container to wait for", op.Name)

Because OpWaitCondition nodes are always ungrouped (empty Group), onNodeError delegates to emitErrorEvent. The default branch there (id = op.ResourceID) produces "wait:db:service_healthy: required dependency "db" has no container to wait for" in the UI — an internal key, not a display name.

Fix: emit a named error event before returning, mirroring the skip path:

exec.compose.events.On(api.Resource{ID: "Service " + op.Name, Status: api.Error, Text: "no container to wait for"})
return fmt.Errorf("required dependency %q has no container to wait for", op.Name)

Or extend emitErrorEvent's switch with a case for op.Name != "" (when Container is nil but Name is set).

Confidence	Score
🟡 moderate	75/100