Repository navigation
feat: github_merge_pr: optional subject/body (gh pr merge --subject/--body), refused on rebase, shown in dry run - #45
Conversation
…-body), refused on rebase, shown in dry run
There was a problem hiding this comment.
QA panel review — PASS
code-review · head e5da11f50442 · formal
Low-risk change: a single test-assertion style nit. The panel found no correctness, security, or logic issues. The one finding is a confirmed nit — a new test asserts only the tail of argv (args[-6:]) while its sibling test in the same diff uses the full-argv pattern. No action needed before merge; fix at author's discretion. Verification confirmed the code matches the quoted evidence and the in-file inconsistency is real.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| ⚪ | nit | tests/test_write_tools.py:465 |
test_merge_pr_merge_with_subject_body_and_delete_branch asserts only the last 6 argv elements (args[-6:]) instead of the full argv, deviating from the repo's s… | confirmed |
findings JSON (machine-readable)
[
{
"file": "tests/test_write_tools.py",
"line": 465,
"severity": "nit",
"category": "conventions",
"claim": "test_merge_pr_merge_with_subject_body_and_delete_branch asserts only the last 6 argv elements (args[-6:]) instead of the full argv, deviating from the repo's stated convention (\"asserts the exact argv\") and the established pattern in the same file.",
"evidence": "assert args[-6:] == [\"--merge\", \"--delete-branch\", \"--subject\", \"a subject\", \"--body\", \"a body\"]",
"verdict": "confirmed",
"note": "Code at PR head line 462 matches the quoted evidence exactly; the sibling test in the same diff uses full-argv assertion (args == [...]), confirming the pattern deviation. The 'stated convention' sub-claim is unverified (no doc fetched) but the in-file inconsistency is clear.",
"line_original": 462,
"line_corrected": true
}
]There was a problem hiding this comment.
Promoting the PASS verdict for head e5da11f50442: all checks terminal-green, zero unresolved review threads. (approve-on-green)
Open findings carried by this approval — non-blocking, but they did not go away:
- nit
tests/test_write_tools.py:465— test_merge_pr_merge_with_subject_body_and_delete_branch asserts only the last 6 argv elements (args[-6:]) instead of the full argv, deviating from the repo's stated convention ("asserts the exact argv
Approving a WARN does not resolve its findings (issue #22).
Summary
github_merge_prnow accepts optionalsubjectandbodyparams (placed afterdelete_branch, beforedry_run/confirm, so every existing call stays valid). When set, they're appended to the gh argv as["--subject", subject]/["--body", body]after--<method>and--delete-branch. Because a single-commit squash takes the commit's subject (not the PR title) — which is how design-system-plugin#48 landed with the wrong subject and skipped its release workflow — this lets callers pin the exact subject that lands on main. A rebase merge ignores these, so passing either withmethod="rebase"returns anErrorbefore any gh call. The dry-run preview now echoes the exact subject/body for operator confirmation, and the no-subject/no-body preview is unchanged. Thepr.mergedevent payload is unchanged and no version bump is included (that ships on a separate release card).Fixes #44