fix(coordinator): honor needs: between background jobs (#454)

[avihut]

Summary

• Reuse the foreground DagGraph machinery in the background coordinator so
  bg→bg needs: is honored at runtime, not just on paper.
• run_single_background_job now returns NodeStatus; a hand-rolled
  wave scheduler in run_all_with_cancel spawns one std::thread::spawn
  per ready node, joins each wave, and cascades DepFailed to dependents.
• DagGraph is reused for cycle / missing-dep detection and dependent
  lookups but not for execution: DagGraph::run_parallel builds on
  std::thread::scope, which can deadlock post-libc::fork() on macOS due
  to inherited malloc-arena state. The wave scheduler avoids that by using
  the same bare-spawn primitive the pre-fix coordinator used.
• Cancelled / failed deps cascade DepFailed to dependents; dep-failed bg
  jobs synthesize a JobStatus::Skipped meta.json so they remain visible
  to daft hooks jobs.

Fixes #454.

Test plan

• Unit: bg_dependent_waits_for_dep_to_finish (regression for Background hook coordinator ignores needs: between background jobs #454)
• Unit: bg_dependent_skipped_when_dep_fails
• Unit: bg_dependent_skipped_when_dep_cancelled
• Unit: bg_cycle_in_needs_returns_error, bg_missing_dep_in_needs_returns_error
• YAML scenario: tests/manual/scenarios/hooks/bg-needs-ordering.yml
• All existing coordinator unit tests still pass (85/85 in coordinator module)
• Full integration matrix (default+gitoxide, bash+yaml): 504 scenarios, 1872 steps, all pass
• mise run fmt:check, mise run clippy, mise run test:unit, mise run test:integration

[avihut]

/claude review

[claude]

Claude finished @avihut's task in 4m 19s —— View job

---

PR Review: fix(coordinator): honor needs: between background jobs

• Gather context: read changed files and git diff
• Review src/coordinator/process.rs — correctness, Rust idioms, safety
• Review tests/manual/scenarios/hooks/bg-needs-ordering.yml — test coverage
• Review docs changes
• Post final review

---

Summary

The fix is sound. The wave-based scheduler correctly implements topological ordering for background jobs, the DFS cascade for transitive DepFailed propagation is correct, and the macOS fork-safety concern (avoiding thread::scope post-fork()) is properly documented and addressed. Test coverage is thorough: unit tests cover the regression, fail-cascade, cancel-cascade, cycle, and missing-dep cases, and the YAML scenario provides end-to-end coverage.

A few things worth discussing before merge:

---

Issues

1. NodeStatus::Skipped arm in wave outcomes is dead code

src/coordinator/process.rs, lines 173–198

match status {
    NodeStatus::Succeeded | NodeStatus::Skipped => {
        // advance dependents
    }
    NodeStatus::Failed => { ... }
    _ => {}
}

run_single_background_job returns only NodeStatus::Succeeded or NodeStatus::Failed (the final if/else at lines 478–482 makes this explicit). The Skipped variant in the first arm is unreachable. It's harmless today, but it implies to readers that a job can return Skipped, and someone may rely on that implication in the future. Consider either removing it or adding a comment explaining it's reserved for future use.

Fix this →

---

2. Cancelled-before-start early return: JobResult.status is Skipped, return value is Failed

src/coordinator/process.rs, lines 279–289

if cancel_all.load(Ordering::Relaxed) {
    results.lock().unwrap().push(JobResult {
        status: NodeStatus::Skipped,   // ← what goes into results
        ...
    });
    return NodeStatus::Failed;          // ← what the DAG cascade sees
}

The semantic split — Skipped in the result, Failed to the wave scheduler — is explained at the bottom of the function (lines 474–482). But that comment is ~190 lines away from the early-return site. A reader of the early-return branch will see the inconsistency without context. A one-line inline comment on the return NodeStatus::Failed would make the intent clear without requiring the reader to scroll. Also note that this early-return path doesn't write a meta.json, so the cancelled-before-start job is invisible to daft hooks jobs — that's pre-existing but slightly surprising given that the PR adds synthesis for DepFailed jobs.

---

3. Wave scheduler serializes independent parallel chains (intentional, but worth documenting)

src/coordinator/process.rs, lines 125–199

The wave scheduler waits for every node in the current wave before advancing, which means:

A (slow, no deps) 
B (fast, no deps) → C

Here C waits for A even though they are completely independent. With a free-running scheduler (like DagGraph::run_parallel), C would start as soon as B finishes. The PR body explains the macOS fork-safety rationale, but the code comment inside the function doesn't mention the trade-off — it only states the reason for using bare spawns, not the implication for independent chains. For a docs/superpowers/specs audience this is probably fine, but a future maintainer might "fix" the serialization and reintroduce the deadlock. A brief comment near the while !ready.is_empty() loop noting "this serializes waves intentionally" would protect the invariant.

---

4. SKILL.md not updated (CLAUDE.md requires this)

SKILL.md lines 374–375 currently say:

Background jobs participate in the DAG. If a foreground job depends on a background job, the background job is promoted to foreground automatically.

This describes the foreground-promotes-background direction, but says nothing about bg→bg needs: ordering being honored at runtime (which was the bug). A reader of SKILL.md today would have no guarantee that needs: between two background-only jobs is respected by the coordinator. CLAUDE.md explicitly requires updating SKILL.md when feature behavior changes. Suggest adding a bullet to the "Key behaviors" list (around line 381) such as:

• needs: between background jobs is honored at runtime — the coordinator schedules them in topological wave order.

Fix this →

---

5. Large planning doc committed to the repo

docs/superpowers/plans/2026-05-02-bg-jobs-honor-needs.md — 1022 lines

This appears to be an agent-generated implementation plan. It's the kind of document CLAUDE.md discourages committing ("Do not create planning, decision, or analysis documents unless the user asks for them"). Is this directory (docs/superpowers/plans/) intentionally part of the public repo? If so, that's fine and no action needed — just confirming intent.

---

Minor observations

• bg-needs-ordering.yml uses python3 for nanoseconds: date +%s%N is available on Linux GNU coreutils and avoids the Python dependency, though python3 is fine for the CI environment this targets.

• bg_dependent_skipped_when_dep_cancelled uses a global temp path (/tmp/should-never-exist-bg-needs-cancel): two parallel test runs could race on this file. Consider tempfile::NamedTempFile or a unique suffix.

• handle.join().unwrap_or(NodeStatus::Failed): solid defensive choice — a panicked worker is treated as Failed, which fires the cascade correctly.

• DAG index → job index alignment: The nodes vec is built from self.jobs.iter() in the same order, so self.jobs[idx] is always the right job for graph-index idx. This implicit alignment is an invariant worth a comment if the jobs collection is ever made mutable after graph construction.

---

Test coverage ✓

The new unit tests are comprehensive and well-structured:

• bg_dependent_waits_for_dep_to_finish — timing assertion using started_at/finished_at timestamps
• bg_dependent_skipped_when_dep_fails — synthesized Skipped meta + side-effect check
• bg_dependent_skipped_when_dep_cancelled — cancel signal cascade + meta check
• bg_cycle_in_needs_returns_error / bg_missing_dep_in_needs_returns_error — DAG validation
• YAML scenario end-to-end regression

All five regression cases from the checklist in the PR body are covered. The existing 85 coordinator tests passing confirms no regressions.