Skip to content

Pipeline Plan inline

Jump to bottom
ezigus edited this page Apr 27, 2026 · 4 revisions

I have enough context to write the full plan. Here it is:

Implementation Plan: Pipeline Bug Fixes (3 bugs)

Root Cause Analysis

Bug 1 — Heartbeat orphan: The heartbeat subshell uses sleep 600 directly. When the subshell is killed with SIGTERM, sleep survives as an orphan because SIGTERM is only delivered to the subshell, not its children. Pattern from commit 1a21406 solves this by backgrounding sleep and using wait $!, so the trap can reach job %1.

Bug 2 — _safe_base_diff false negative: git rev-parse --verify main fails in GHA because main is only available as origin/main. The function falls back to HEAD~5, which only sees .claude/ artifact commits, so the PR stage sees zero real changes and rejects the branch.

Bug 3 — Drift detection silent skip: detect_plan_drift has its own git rev-parse --verify "$_base" guard that returns 0 (skip) for the same reason as Bug 2. Since drift detection calls _safe_base_diff (which will be fixed), the guard itself just needs to accept origin/$_base before giving up.

Files to Modify

File Change
.github/workflows/shipwright-pipeline.yml Fix heartbeat subshell (lines 625–631)
scripts/lib/pipeline-stages.sh Fix _safe_base_diff with origin/$branch fallback
scripts/lib/pipeline-stages-review.sh Fix drift detection guard
scripts/sw-lib-pipeline-stages-test.sh Add TDD tests for _safe_base_diff GHA env
scripts/sw-lib-pipeline-stages-review-test.sh Add TDD tests for drift detection GHA env
scripts/sw-heartbeat-orphan-test.sh (new) Smoke test for heartbeat SIGTERM/orphan

Alternatives Considered

Bug 2:

  • Alt A (chosen): Add origin/$branch middle tier in _safe_base_diff. Blast radius: zero — callers unchanged, one function fixed for all 14 callers.
  • Alt B: Fix each of the 14 callers. Higher blast radius, more diff surface, error-prone.

Bug 3:

  • Alt A (chosen): Update the existing guard to accept origin/$_base. Minimal — one if condition change.
  • Alt B: Remove the guard entirely and let _safe_base_diff handle it. Riskier because the guard also protects the fallback git diff --name-only path on line 53.

Implementation Steps

Phase 1 — Write tests (TDD first)

  1. Create scripts/sw-heartbeat-orphan-test.sh — smoke test that:
    • Launches a subshell with the old pattern, sends SIGTERM, asserts orphan sleep remains
    • Launches a subshell with the new pattern, sends SIGTERM, asserts no orphan sleep
  2. Add to scripts/sw-lib-pipeline-stages-test.sh — new test section that:
    • Sets up a git repo where main is NOT a local ref (only origin/main)
    • Calls _safe_base_diff and asserts it returns the origin/main...HEAD diff, not HEAD~5
    • Asserts a stderr warning is printed when HEAD~5 fallback IS used
  3. Add to scripts/sw-lib-pipeline-stages-review-test.sh — new test section that:
    • Sets up same GHA-style repo (no local main, only origin/main)
    • Calls detect_plan_drift and asserts it runs (does NOT return early/skip)
    • Verifies drift warnings appear correctly even without a local main

Phase 2 — Implement fixes

  1. Fix .github/workflows/shipwright-pipeline.yml lines 625–631: 
    (trap 'kill %1 2>/dev/null; exit 0' TERM; while true; do sleep 600 & wait $!; ...done) &
  2. Fix scripts/lib/pipeline-stages.sh _safe_base_diff (lines 147–151):
    • After failing git rev-parse --verify "$branch", try git rev-parse --verify "origin/$branch"
    • If origin/$branch exists, use git diff "origin/${branch}...HEAD"
    • Only fall to HEAD~5 if both fail; add warn to stderr in that case
  3. Fix scripts/lib/pipeline-stages-review.sh guard (lines 47–48):
    • Change the condition to also accept origin/$_base before returning 0
    • Update the comment to document the new three-tier behavior

Phase 3 — Verify

  1. Run the new heartbeat orphan smoke test
  2. Run scripts/sw-lib-pipeline-stages-test.sh and verify new tests pass
  3. Run scripts/sw-lib-pipeline-stages-review-test.sh and verify new tests pass
  4. Run npm test for full regression

Task Checklist

  • Task 1: Create scripts/sw-heartbeat-orphan-test.sh with old-pattern (orphan) + new-pattern (clean) smoke tests
  • Task 2: Add GHA-env _safe_base_diff tests to scripts/sw-lib-pipeline-stages-test.sh
  • Task 3: Add GHA-env detect_plan_drift tests to scripts/sw-lib-pipeline-stages-review-test.sh
  • Task 4: Fix heartbeat subshell in .github/workflows/shipwright-pipeline.yml
  • Task 5: Fix _safe_base_diff in scripts/lib/pipeline-stages.sh with origin/$branch middle tier + HEAD~5 stderr warn
  • Task 6: Fix drift detection guard in scripts/lib/pipeline-stages-review.sh + update comment
  • Task 7: Run scripts/sw-heartbeat-orphan-test.sh — all tests pass
  • Task 8: Run scripts/sw-lib-pipeline-stages-test.sh — new tests pass, no regressions
  • Task 9: Run scripts/sw-lib-pipeline-stages-review-test.sh — new tests pass, no regressions
  • Task 10: Run full npm test — no regressions

Test Pyramid Breakdown

  • Unit (shell): 6 tests — _safe_base_diff with local ref, with origin/ ref, with neither (HEAD~5 warn); detect_plan_drift with local main, with origin/main only
  • Smoke: 2 tests — heartbeat SIGTERM with old pattern (orphan present), new pattern (no orphan)
  • Regression (npm test): full suite to verify no regressions

Testing Approach

Bug 1 — sw-heartbeat-orphan-test.sh:

# Old pattern: launch subshell, kill it, check for orphan sleep
( while true; do sleep 600; done ) &
OLD_PID=$!; sleep 0.1; kill $OLD_PID 2>/dev/null; sleep 0.2
assert [[ $(pgrep -P $OLD_PID sleep 2>/dev/null | wc -l) -gt 0 ]]  # orphan exists

# New pattern: same, verify no orphan
( trap 'kill %1 2>/dev/null; exit 0' TERM; while true; do sleep 600 & wait $!; done ) &
NEW_PID=$!; sleep 0.1; kill $NEW_PID 2>/dev/null; sleep 0.2
assert [[ $(pgrep -P $NEW_PID sleep 2>/dev/null | wc -l) -eq 0 ]]  # no orphan

Bugs 2 & 3 — Set up a bare clone with origin/main only:

git init bare_remote && git clone bare_remote proj
cd proj && git checkout -b feat/work  # no local 'main', only origin/main
# Now call _safe_base_diff / detect_plan_drift and assert correct behavior

Definition of Done

  • Heartbeat subshell in the workflow uses trap 'kill %1 2>/dev/null; exit 0' TERM; sleep N & wait $! pattern
  • _safe_base_diff returns origin/$branch...HEAD diff when local $branch doesn't exist but origin/$branch does
  • _safe_base_diff prints a stderr warning when falling back to HEAD~5
  • detect_plan_drift does NOT silently return 0 when origin/$_base is available
  • All 3 new test files/sections pass
  • npm test passes with no regressions
  • No callers of _safe_base_diff are modified

Clone this wiki locally