ADR-254: Wire ReviewThread model through all pipeline steps

[jodavis-claude]

Work item

ADR-254: Update all pipeline step classes in dev_team.py to replace the PR-URL-based review handoff with the ReviewThread data model — wiring UUID assignment, thread merging, and structured markers through every affected step.

Changes

• plugins/dev-team/scripts/dev_team.py — Pipeline step classes updated to use ReviewThread model throughout:
  • PipelineContext: added signoff_notes: str = "" field with <!-- section:Signoff Notes --> save/load
  • ReviewStep: removed prepare-pr-details block; UUID assignment for parsed thread dicts; saves ctx.review_threads and ctx.review_notes; emits [DEV-TEAM] Review ready: <status>
  • FixPrStep: added __init__(self, context_path) constructor; passes unresolved threads as $REVIEW_THREADS and $ISSUES: ""; merges Developer's returned thread list discarding resolved field; calls ctx.save()
  • SignoffStep: added __init__(self, context_path) constructor; uses $BASE_BRANCH / $REVIEW_THREADS substitutions; _run_reviewer_signoff() returns 4-tuple; merges sign-off threads (append-not-replace); stores body in ctx.signoff_notes; emits [DEV-TEAM] Signoff: <status>; calls sys.exit(1) in main thread on failure
  • ValidateStep: added __init__(self, context_path) constructor; sets ctx.first_push_done = True, calls ctx.save(), emits [DEV-TEAM] First push complete — only when first_push_done was False
  • ImplementStep: emits [DEV-TEAM] Implementation started at top of run() and [DEV-TEAM] Implementation complete before every return (both on resume)
  • DebugStep: emits [DEV-TEAM] Debug complete after debugger agent returns
  • FixStep: added "$REVIEW_THREADS": "" to substitutions dict
  • Imports: added from uuid import uuid4
• plugins/dev-team/scripts/test_dev_team.py — 17 new unit tests across 4 new test classes:
  • TestPipelineContextSignoffNotes (5 tests): default empty, save unconditional, round-trip empty/with-content, load missing key
  • TestFixPrStepThreadMerge (3 tests): comments replaced on match, resolved preserved from existing thread, unknown IDs silently discarded
  • TestSignoffStepThreadMerge (4 tests): comments appended not replaced, resolved updated, empty comments only updates resolved, new threads added with UUIDs
  • TestSignoffStepExitGuard (2 tests): exits when reviewer agent raises, exits when reviewer returns empty threads
  • TestValidateStepFirstPush (3 tests): sets first_push_done to True, marker emitted when False, marker not emitted when already True

All 52 tests pass (35 pre-existing + 17 new).

Design decisions

• ReviewStep $PR_URL gap: Removed "$PR_URL": ctx.pr_url from substitutions entirely (not bridged with ""). The reviewer-review.md skill still contains $PR_URL until ADR-255 lands; unit tests don't invoke the skill so no test failures result.
• signoff_notes scope: This field is required by SignoffStep but was absent from ADR-253's exit criteria. Added it as part of ADR-254 to unblock compilation, mirroring the review_notes pattern exactly.
• Thread merge semantics differ between steps: FixPrStep replaces .comments wholesale (Developer echoes full history); SignoffStep appends new comments only (Reviewer returns delta). This matches the spec's ownership invariant — only Reviewer sets resolved: true.
• sys.exit(1) placement in SignoffStep: Called in the main thread (not in the worker future) per spec requirement. Guard triggers on reviewer_ok=False OR empty thread list, before the failures list check.
• Single save per SignoffStep invocation: ctx.save() called once inside SignoffStep on both approval and changes_requested paths; the outer DevTeamPipeline.run() save is idempotent.

[jodavis-claude]

ADR-254 Review

One Priority 1 (correctness) issue found: deserializes reviewer-agent output with direct dict key access (, ) that will raise an unhandled if the agent returns malformed threads. All other exit criteria are met.

Exit criteria checklist:

• ✅ : PR-creation block removed; UUIDs assigned; / saved; emits
• ✅ : constructor arg; + ; thread-merge discards ;
• ✅ : / ; 4-tuple return; sign-off thread merge; ; ; in main thread on failure
• ✅ : False→True; before marker; marker not emitted on resume
• ✅ : at top; before both return paths
• ✅ : emitted; not emitted on early-return path
• ✅ : in substitutions dict
• ✅ : field with <!-- section:Signoff Notes --> sentinel; unconditional Review Notes / Review Threads sections
• ✅ New tests: , , ,

Known gap (not blocking): still contains the hardcoded PR URL literal that will not be substituted until ADR-255 — developer chose option (b) from Known Ambiguity #2, acceptable per spec.

[jodavis-claude]

ADR-254 Review

One Priority 1 issue: ReviewStep.run() uses t["filePath"] and t["lineNumber"] with direct key access -- unhandled KeyError if the reviewer agent returns malformed threads. All other exit criteria are met.

Exit criteria: ReviewStep (PR-creation removed, UUIDs, threads/notes saved, marker) [x], FixPrStep (context_path, REVIEW_THREADS, merge discards resolved, save) [x], SignoffStep (BASE_BRANCH/REVIEW_THREADS, 4-tuple, merge, signoff_notes, sys.exit in main thread) [x], ValidateStep (first_push_done, save-before-marker, no double emit) [x], ImplementStep (started/complete both paths) [x], DebugStep (Debug complete, not early) [x], FixStep (REVIEW_THREADS="") [x], PipelineContext (signoff_notes + section) [x], Tests (TestFixPrStepThreadMerge, TestSignoffStepThreadMerge, TestSignoffStepExitGuard, TestValidateStepFirstPush) [x]

Known gap (not blocking): reviewer-review.md hardcoded PR URL not substituted until ADR-255 -- option (b) from Known Ambiguity #2.

[jodavis-claude]

Sign-off review for ADR-254.

Prior threads (2): Both P1 correctness issues about direct key access (t["filePath"], t["lineNumber"]) in ReviewStep were addressed in commit 3d24db4. The fix correctly uses t.get("filePath", "") and t.get("lineNumber", 0), consistent with SignoffStep's new-thread path and PipelineContext.load(). Both threads resolved.

New issues in modified files: None. Commit 3d24db4 is a targeted, minimal fix with no regressions. All 52 tests pass.

Outcome: Approved — all review threads resolved, no new blocking issues found.