hackbot: submit Phabricator revisions as WIP drafts#6309
Conversation
Hackbot produces candidate fixes, so revisions should land as WIP/draft (Phabricator changes-planned) for a human to review before formal review, instead of going straight to needs-review and pinging reviewers. Mirror moz-phab's --wip exactly: add a plan-changes transaction (in a separate follow-up edit when the revision is already changes-planned), prefix the title with "WIP: ", and omit reviewers; the local:commits summary/message inherit the WIP title and empty reviewers. Non-WIP updates request review when re-activating a non-review revision. WIP is a hackbot-api policy, not an agent choice: the applier injects the flag into the submit_patch params at dispatch (SUBMIT_PATCHES_AS_WIP, hardcoded for now, later from per-agent hackbot.toml). The submit_patch tool drops its wip and reviewers parameters, simplifying the agent-facing signature.
There was a problem hiding this comment.
Pull request overview
This PR changes Hackbot’s Phabricator submission flow so newly submitted revisions land as WIP/draft (changes-planned) without requesting reviewers, matching moz-phab’s --wip behavior and avoiding premature reviewer notifications.
Changes:
- Inject a hackbot-api policy flag (
wip=True) forphabricator.submit_patchdispatches and add tests covering injection behavior. - Update the Phabricator submit handler to (by default) prefix titles with
WIP:, applyplan-changes, omit reviewers, and request review only when re-activating a non-review revision in non-WIP mode. - Remove “reviewers” from the recorded action schema and adjust runtime tests accordingly.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| services/hackbot-api/app/actions_applier.py | Injects wip into phabricator.submit_patch params at dispatch time via a hardcoded policy constant. |
| services/hackbot-api/tests/test_actions_applier.py | Extends coverage to assert WIP injection for Phabricator actions and absence for non-Phabricator actions. |
| libs/hackbot-runtime/hackbot_runtime/actions/phabricator.py | Removes the reviewers parameter from the recording tool and stops recording reviewers in params. |
| libs/hackbot-runtime/tests/test_phabricator_actions.py | Updates expectations for recorded params and removes reviewers-default test. |
| libs/hackbot-runtime/hackbot_runtime/actions/handlers/phabricator_handler.py | Implements WIP title normalization, plan-changes/request-review transaction logic, and forces empty reviewers in arc commit message reconstruction. |
| libs/hackbot-runtime/tests/test_phabricator_handler.py | Updates and adds tests for WIP-by-default behavior, explicit non-WIP behavior, local:commits title/message expectations, and the “second edit” plan-changes case. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@suhaibmujahid could/should we use the Phab API to automatically have the developer who requested the run commandeer the revision? |
We could! But whether we should depends on how we want the flow to work. My guess is that leaving it open (not commandeered) lets anyone pick it up, which might actually be what we want to encourage (see a Hackbot patch, feel free to take it over). But this is one of those things that's hard to get right before we've actually put it into practice. |
Resolves #6308
Hackbot produces candidate fixes, so revisions should land as WIP/draft (Phabricator changes-planned) for a human to review before formal review, instead of going straight to needs-review and pinging reviewers.
Mirror moz-phab's --wip exactly: add a plan-changes transaction (in a separate follow-up edit when the revision is already changes-planned), prefix the title with "WIP: ", and omit reviewers; the local:commits summary/message inherit the WIP title and empty reviewers. Non-WIP updates request review when re-activating a non-review revision.