Split phabricator.submit_patch into submit_patch and update_patch#6382
Draft
suhaibmujahid wants to merge 1 commit into
Draft
Split phabricator.submit_patch into submit_patch and update_patch#6382suhaibmujahid wants to merge 1 commit into
suhaibmujahid wants to merge 1 commit into
Conversation
One tool that both created and updated a revision meant the agent had to get an optional `revision_id` exactly right: omitting it silently created a new revision, and passing a guessed one silently updated a stranger's. Now `phabricator.submit_patch` creates (bug_id + required title + summary) and `phabricator.update_patch` updates (bug_id + required revision_id, with title and summary optional and defaulting to the revision's current values). Each tool only accepts the parameters its own case needs, so the schema itself rules out the wrong call. Apply-side stays one handler (renamed PatchSubmissionHandler) registered for both types, since the Conduit calls are identical apart from the revision id. Places gated on "this run submits a patch" now check the shared PATCH_ACTION_TYPES set: the diff artifact built in publish_changes and the WIP policy injected in hackbot-api. Also corrects the ref-placeholder example in both tool docstrings: the applied result exposes revision_url, not url.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
phabricator.submit_patchdid double duty: create a revision, or update one if an optionalrevision_idwas passed. That put the create-vs-update decision entirely on the agent getting one optional argument right. Omit it by mistake and a new revision appears instead of the intended update; pass a guessed number and an unrelated revision gets a new diff.What changed
Two agent-facing tools, each with only the parameters its case needs:
phabricator.submit_patch(create)bug_id,title,reasoningsummary,refphabricator.update_patch(update)bug_id,revision_id,reasoningtitle,summary,refsubmit_patchno longer accepts arevision_idat all, so it cannot update anything. Itstitleis now required by the schema rather than by a runtimeToolError.update_patchrequiresrevision_id;title/summaryare omitted to keep the revision's current values (the handler already fetched them, this just makes it the documented default).PatchSubmissionHandler, registered for both action types: the Conduit calls are identical apart from the revision id.PATCH_ACTION_TYPESset: the diff artifact built inpublish_changes, and the WIP policy injected in hackbot-api.Drive-by: both tool docstrings advertised
{{actions.<ref>.url}}, but the applied result exposesrevision_url. Corrected.Testing
libs/hackbot-runtime(148 passed) andservices/hackbot-api(49 passed) suites pass, plus ruff check/format. New coverage: the two tools' recorded params and schemas (revision_idabsent from create, required for update), both types routing to the handler, and both types triggering the diff artifact and the WIP injection.