ADR-402: add user-approve-posts skill and work-with-pr post-comment operation - #190
ADR-402: add user-approve-posts skill and work-with-pr post-comment operation#190jodavis-claude wants to merge 2 commits into
Conversation
…s comments Adds a new bare, mechanical operation to work-with-pr for posting a plain, freestanding status comment to a PR, reusing the same add_issue_comment tool (and gh CLI fallback) work-with-github-issues already documents for plain issue comments, since GitHub treats a PR number as an issue number. Calls message-attribution first, matching the existing "Posting a review" and thread-reply operations. This is the sole dependency user-approve-posts (ADR-402, next commit) needs for its pr-comment dispatch channel.
New Orchestrator skill that owns the full approve/edit/discuss/reject/attribute/dispatch/cleanup flow for a list of proposed posts (PostEntry: content, channel, target, file_path). First pass batches entries through AskUserQuestion (up to 4 at a time, four options each); deferred "Discuss this" entries are revisited one at a time as an open-ended conversation in a second pass, following document-discussion's precedent. Dispatches approved entries to work-with-Jira-tasks / work-with-github-issues / work-with-pr's new post-comment operation by channel, appending the fixed reviewed-by attribution line, and deletes each entry's file (caller-supplied or self-created temp file) the moment it resolves. Resolves <user-name> once per call via Jira's atlassianUserInfo, falling back to gh api user, falling back to git-repo.user-alias. Computes <repo-slug> and the user-approve-posts-tmp temp directory via a python3 -c one-liner reaching get_repo_slug() in workflow-orchestrate/scripts, matching the existing relative-import convention and honoring the DEV_TEAM_STATE_DIR/GIT_REMOTE_URL_OVERRIDE test seams (smoke-tested manually). No automated test harness exists for prose-only agent skills in this repo (missing-test-harness); verified by self-review against the exit criteria instead, per the task brief's explicit guidance.
build-and-test: Python test resultsStatus: ✅ Passed Test log |
jodavis-claude
left a comment
jodavis-claude
left a comment
There was a problem hiding this comment.
Reviewed ADR-402: work-with-pr's new post-comment operation, and the new user-approve-posts skill. All exit criteria from the task brief are satisfied at the prose level (PostEntry shape, user-name resolution order, AskUserQuestion batching, file-based edit flow, two-pass discuss handling, reviewed-by attribution appended as its own line, dispatch table for all four channels, and final posted/rejected counts). Cross-checked against the ADR-400 queue_post.py CHANNELS/target naming, work-with-Jira-tasks/work-with-GitHub-issues operation names, message-attribution's composition rule, and the sibling-skill relative-path convention used elsewhere in this repo (dev-spec-create-work-items) -- all consistent.
One correctness/fault-tolerance gap found and flagged inline: the cleanup step doesn't distinguish a successful dispatch from a failed one, so a dispatch error could silently delete an entry's only remaining copy with no way to retry. Requesting that be addressed before sign-off.
No test harness exists for prose-only skills in this repo (confirmed via missing-test-harness), so self-review against the exit criteria is the right verification approach here, consistent with the taxonomy for Wrapper/Orchestrator components.
|
|
||
| Whether the entry was approved and dispatched, or rejected and discarded, delete whatever file | ||
| it was using — its caller-supplied `file_path`, or a temp file this skill created in step 2 — | ||
| the moment the entry resolves. Do not batch deletions until the end. |
There was a problem hiding this comment.
Priority 2 (correctness / fault tolerance): this cleanup step doesn't distinguish a successful dispatch from a failed one. If the downstream posting call errors partway through (auth failure, rate limit, network blip), the entry's file -- often the only remaining copy of that content -- still gets deleted per the literal wording ("delete ... the moment the entry resolves"), silently losing the post with no way to retry.
Suggest adding explicit failure handling: on a dispatch error, keep the file, surface the error to the user, and don't count that entry toward the final counts -- leave it available to retry instead of deleting it. The Step 5 return value should also clarify that its count reflects successful posts, not merely attempted ones.
Review SummaryOutcome: Changes requested Issue found (Priority 2 — Correctness): Recommended fix: Keep the file on dispatch error, surface the error to the user, and leave entries available for retry. The return value should clarify it counts successful posts, not attempted dispatches. Everything else: Clean. All other components verified correctly:
|
2 participants
Work item: ADR-402 — Add a
user-approve-postsskill that owns the full batch approve/edit/discuss/reject/attribute/dispatch/cleanup flow for a list of proposed posts, and add a "post-comment" operation towork-with-prso a plain status comment can be posted to a PR.Changes:
plugins/dev-team/skills/work-with-pr/SKILL.md— new## Posting a plain comment/### post-commentoperation: given a PR URL and body text, callsmessage-attributionfirst, thenmcp__plugin_github_github__add_issue_comment(PR number treated as issue number), falling back togh pr comment <pullNumber> --repo <owner>/<repo> --body "<text>". Reuses the existing "Extracting PR details from a URL" logic; existing structured-review, thread-reply, and hand-off-operations mechanics are unmodified.plugins/dev-team/skills/user-approve-posts/SKILL.md— new skill implementinguser_approve_posts(posts: list[PostEntry]) -> {"posted": int, "rejected": int}. Resolves<user-name>once per call (JiraatlassianUserInfo→gh api user --jq .login→git-repo.user-alias). First pass batches entries throughAskUserQuestion(up to 4 at a time, four options: Approve as-is / Edit and approve / Discuss this / Reject). "Edit and approve" re-readsfile_pathwhen set, or creates a fresh temp file under~/.dev-team/<repo-slug>/user-approve-posts-tmp/when unset. "Discuss this" entries are deferred to a one-at-a-time open-ended second pass modeled ondocument-discussion. Every approved entry gets the fixed"Written by Claude Code and reviewed by <user-name> before posting."line appended and is dispatched bychanneltowork-with-Jira-tasks,work-with-GitHub-issues, orwork-with-pr's newpost-commentoperation. Each entry's file is deleted the instant it resolves (posted or rejected).Design decisions:
post-commentwas placed as its own new## Posting a plain commenttop-level section (with a### post-commentheading) rather than as a fourth entry inside## Hand-off operations, to avoid invalidating that section's "these three operations" framing sentence — a smaller, more conservative diff.<user-name>resolution tries JiraatlassianUserInfo, thengh api user --jq .login, then falls back togit-repo.user-alias, rather than branching on project type — a simpler strictly-fallback reading of the spec.gh api useris documented inline inuser-approve-postsrather than as a newwork-with-GitHub-issuesoperation, since that extension is outside ADR-402's Component Breakdown.<repo-slug>and the temp-dir root are computed via apython3 -cone-liner reachingget_repo_slug()inworkflow-orchestrate/scripts/get_context_path.py, matching the existing relative-import convention used elsewhere in the repo; both env-var overrides (GIT_REMOTE_URL_OVERRIDE,DEV_TEAM_STATE_DIR) were manually smoke-tested.missing-test-harness.Testing completed:
SKILL.mdfiles against the task brief's exit criteria (Wrapper-tierpost-commentverified by visual inspection; Orchestrator-tieruser-approve-postsverified the same way, consistent with other prose-only orchestrator skills in this repo).<repo-slug>and temp-dirpython3 -cone-liners againstGIT_REMOTE_URL_OVERRIDEandDEV_TEAM_STATE_DIRoverrides, confirming correct output.