ADR-363: Trim final-sign-off to near-no-op; split work-with-pr hand-off ops - #99
ADR-363: Trim final-sign-off to near-no-op; split work-with-pr hand-off ops#99jodavis-claude wants to merge 1 commit into
Conversation
… split out final-sign-off now reports hand-off readiness only (writes Handoff Result); promote/assign/request-review is performed afterward by after-hand-off instructions via run-event-hooks. work-with-pr's fixed 3-step hand-off sequence becomes three independently-callable operations: convert-to-ready, request-review, assign-issue (via work-with-Jira-tasks). Removed all REVIEW_ASSIGNEE_EMAIL references; reviewer/assignee identity is now a literal argument the caller supplies.
jodavis-claude
left a comment
There was a problem hiding this comment.
Reviewed against ADR-363's exit criteria in _spec_WorkflowEventHooks.md and the task brief.
Summary: Clean, tightly-scoped change. Both files match the exit criteria exactly:
final-sign-off/SKILL.mdis reduced to a single near-no-op step, with itsdescription:reworded to stop overpromising.work-with-pr/SKILL.md's hand-off section is replaced by three independently-named, mechanical operations (convert-to-ready,request-review,assign-issue), each taking the reviewer/assignee identity as a literal argument.- No
REVIEW_ASSIGNEE_EMAILreference remains in either file (confirmed via repo-wide grep — the only remaining hits are in the spec doc's narrative prose, which is out of scope here). - Operation names (
convert-to-ready,request-review) matchrun-event-hooks/SKILL.md's existing dispatch table verbatim, so no follow-up rename is needed. - The other
work-with-prsections (Reading the PR,Posting a review,Responding to and resolving threads) are untouched, andreview-sign-off,review, andfix-pronly reference those preserved sections — confirmed nothing broke for their callers. HandoffStepdispatchesfinal-sign-offviadev-team:developer(full Jira+GitHub access), so the newassign-issueoperation (viawork-with-Jira-tasks) is reachable in the one place it's used today.- Verification approach (manual Gherkin walkthrough, no new unit tests/fixtures) matches the Wrapper classification in
component-taxonomy/SKILL.mdand the spec's own "inspection-based" verification note.
No Priority 1-4 (requirements/correctness/security/performance) issues found. No inline comments needed — the diff is small and every criterion is met by direct inspection.
One non-blocking observation (already flagged by the author in the brief, not new): work-with-pr's assign-issue operation currently has no live caller going through it by name, since run-event-hooks's dispatch table resolves "Assign work item to " directly against work-with-Jira-tasks instead. That's a pre-existing inconsistency between run-event-hooks (out of this task's ownership) and the spec text, not something this PR needs to fix.
Approving.
Work item
ADR-363: Trim
final-sign-offdown to a near-no-op status report andwork-with-pr's hand-off section down to bare, individually-callable mechanical operations, and remove all remainingREVIEW_ASSIGNEE_EMAILreferences — because promote/assign/request-review behavior now lives inafter-hand-offconfig instructions (shipped by ADR-360/ADR-361), executed generically byrun-event-hooks, not hardcoded inside these two skills.Changes
plugins/dev-team/skills/final-sign-off/SKILL.md— deleted the former steps 1–5 (convert PR to ready, look up reviewer via$REVIEW_ASSIGNEE_EMAIL, assign Jira issue, request GitHub review, add Jira comment). Replaced with a single near-no-op step that writes a short, non-empty confirmation as the skill's own output, whichworkflow-workerplaces into theHandoff Resultcontext section. Reworded the frontmatterdescription:and lead-in paragraph to state the promote/assign/request-review work is now performed by the pipeline event'safter-hand-offinstructions viarun-event-hooks.argument-hint: <pr_url> <work-item-id>left unchanged sinceHandoffStep.get_actions()still passes both.plugins/dev-team/skills/work-with-pr/SKILL.md— replaced the "Requesting human review (approved sign-off only)" section (the fixed 3-step numbered sequence ending in$REVIEW_ASSIGNEE_EMAIL) with a new "Hand-off operations" section containing three separately-headed, independently-callable operations:convert-to-ready,request-review, andassign-issue(the last viawork-with-Jira-tasks'slookupJiraAccountIdtheneditJiraIssue). The$REVIEW_ASSIGNEE_EMAILlookup is gone entirely — every operation now takes the reviewer/assignee identity as a literal argument the calling instruction supplies. All other sections ("Extracting PR details from a URL", "Reading the PR", "Posting a review", "Responding to and resolving threads") were left untouched, sincereview-sign-off,review, andfix-prdepend on them.Design decisions
Wrapperin the spec's Component Breakdown table (thin call-through, visual inspection sufficient percomponent-taxonomy), so verification is inspection-based: the exit criteria's Gherkin scenario was walked through manually against the updated skill text and this repo's own.dev-team/config.yamlafter-hand-offinstructions rather than built as an automated fixture harness.work-with-pr'sassign-issueoperation is documented and independently callable per the spec's literal Component Breakdown text, even though today's only caller (run-event-hooks, out of this task's ownership) currently dispatches Jira assignment directly againstwork-with-Jira-tasksrather than by this name. Flagged as a non-blocking known ambiguity.final-sign-off's old behavior, rather than touching them here.Testing completed
No dedicated unit tests: both components are
Wrapper-classified (thin, prose-only call-throughs), and percomponent-taxonomy/implement-directguidance no dedicated unit test is written for this tier — visual inspection is the appropriate substitute. No new Gherkin feature file: the task brief's own exit-criteria scenario ("Hand-off promotes the PR and assigns the configured reviewer") was designated for manual walkthrough rather than an automated fixture harness (no such harness convention exists yet for prose-onlyWrapperskills in this repo). The manual walkthrough traced everyThenclause against the updated skill text and this repo's realafter-hand-offconfig and confirmed all pass, with noREVIEW_ASSIGNEE_EMAILread anywhere.