Adds pre-push readiness skill#11935
Conversation
| ```bash | ||
| dart run script/tool/bin/flutter_plugin_tools.dart \ | ||
| format --run-on-changed-packages | ||
| ``` |
There was a problem hiding this comment.
Just saw that we could also use the --set-exit-if-changed option. But we could format directly since it might help the developer. @LouiseHsu what do you think?
There was a problem hiding this comment.
I think what you have is good already!
There was a problem hiding this comment.
I think it depends on what you want the scope of this skill to be. My gut says that the skill should not format but instead should say that formatting changes were found. That way one of the evals we can run is "did any files change when running this skill" and enforce that the answer is no. But that only makes sense if the skills "check pre push" skill and not a "prepare for push" skil. So it comes down to a scope question.
Second we are designing this as a system. The precommits should handle formatting(and analyze) so maybe the right answer is to do no formatting checks.
| @@ -0,0 +1,184 @@ | |||
| --- | |||
| name: "pre-push-skill" | |||
| description: "A comprehensive pre-push checklist for contributing to the flutter/packages repository." | |||
There was a problem hiding this comment.
So from what I remember, skills are invoked only based off the description and the name. So maybe you can make the desc more detailed? something like
"Executes the required pre-push steps for the flutter/packages repository. Call this tool immediately whenever the user asks to push, requests a code review before committing, or wants to validate their local changes for a pull request. Do NOT use this tool if the user is working in flutter/flutter or flutter/engine."
There was a problem hiding this comment.
I see. Good call, I like your description! Updated it.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds a new markdown document, SKILL.md, defining the pre-push validation steps and checklist for the flutter/packages repository. The review feedback suggests two improvements to the instructions: first, recommending that developers commit rather than stash their changes during the initial working tree check to ensure latest changes are validated; and second, clarifying that any auto-generated changes in the final check should be committed rather than discarded.
| @@ -0,0 +1,184 @@ | |||
| --- | |||
| name: "pre-push-skill" | |||
| description: "Executes the required pre-push steps for the flutter/packages repository. Call this tool immediately whenever the user asks to push, requests a code review before committing, or wants to validate their local changes can become a pull request. Do NOT use this tool if the user is working in flutter/flutter or flutter/engine." | |||
There was a problem hiding this comment.
I am not sure this should trigger on request for "code review before committing". Consider instead
| description: "Executes the required pre-push steps for the flutter/packages repository. Call this tool immediately whenever the user asks to push, requests a code review before committing, or wants to validate their local changes can become a pull request. Do NOT use this tool if the user is working in flutter/flutter or flutter/engine." | |
| description: "Executes the required pre-push steps for the flutter/packages repository. Call this tool immediately whenever the user asks to push, asks if we/you are ready to push, or wants to validate their local changes can become a pull request. [Do NOT use this tool if the user is working in flutter/flutter or flutter/engine](https://github.com/flutter/flutter/pull/188373)." |
Why did you need "flutter/flutter#188373"? This skill should be loaded locally so it should not leak to those environments.
There was a problem hiding this comment.
Accepted minus the notes on engine/framework
| ## 3. Format Code | ||
|
|
||
| Consistent code style is required for all pull requests. | ||
| The repository uses auto-formatters (like `dart format`, `clang-format`) |
There was a problem hiding this comment.
Not not but maybe later we should reference the check-readiness skill to ensure the formatters are on path and acceptable.
| ```bash | ||
| dart run script/tool/bin/flutter_plugin_tools.dart \ | ||
| format --run-on-changed-packages | ||
| ``` |
There was a problem hiding this comment.
I think it depends on what you want the scope of this skill to be. My gut says that the skill should not format but instead should say that formatting changes were found. That way one of the evals we can run is "did any files change when running this skill" and enforce that the answer is no. But that only makes sense if the skills "check pre push" skill and not a "prepare for push" skil. So it comes down to a scope question.
Second we are designing this as a system. The precommits should handle formatting(and analyze) so maybe the right answer is to do no formatting checks.
|
|
||
| ```bash | ||
| dart run script/tool/bin/flutter_plugin_tools.dart \ | ||
| dart-test --run-on-changed-packages |
There was a problem hiding this comment.
again this is a place where we could simplify the command if we scoped it to camera_android_camerax
There was a problem hiding this comment.
This also has the semi dangerous assumption that main was not broken and that we are merging back into main. Those assumptions are probably fine for a first version of the skill but we might want to articulate a more nuanced version.
There was a problem hiding this comment.
Updated this to include more nuance. We could do a more technical step where we have the agent check if it was failing before but I consider that a bit out of scope.
|
|
||
| ## 8. Final Clean Working Tree Check | ||
|
|
||
| Before pushing, ensure that all your fixes, formatting changes, |
There was a problem hiding this comment.
Lets make a call on if this skill is only about evaluating if the changes are ready to push or if its scope it to actually make the changes.
There was a problem hiding this comment.
Meta comments:
- We need to decide if file changes are part of this skill or if this skill has no file changes and instead the agents responds to failures like a test.
There was a problem hiding this comment.
Decided that this should just be a skill that verifies the state of code versus applies fixes. We can change that later on but I don't think we should introduce the complexity of fixes for now.
|
@reidbaker Re-wrote this quite a bit to focus this skill on verifying code is ready to push versus making automatic fixes and only on the |
| name: "pre-push-skill" | ||
| description: "Executes the required pre-push steps for the flutter/packages repository. Call this tool immediately whenever the user asks to push, asks if we/you are ready to push, or wants to validate their local changes can become a pull request." | ||
| --- | ||
|
|
There was a problem hiding this comment.
One thing this skill does not have is a list of assumptions about what is available on the path. Not a requirement for now but I wanted to point that out since I noticed. Here is an example from flutter/flutter. https://github.com/flutter/flutter/blob/master/.agents/skills/flutter-pr-checks-finder/SKILL.md#prerequisites
There was a problem hiding this comment.
Added gh because we require it for making a PR :)
| give the user a quote from the testing documentation | ||
| on what type of test is required for their changes. | ||
| Beyond the rubric, if you think the change does not meet | ||
| the documented quality bar, tell the user |
There was a problem hiding this comment.
non blocking. We should standardize on how we refer to the person operating the agent/skill/chat and document that in the readme for this folder.
There was a problem hiding this comment.
Added a note in the README!
| (for example, `[camera_android] Fix crash` | ||
| or `[camera_android, camera_android_camerax] Fix crash` | ||
| if both `camera_android` and `camera_android_camerax` were modified). | ||
| - BODY is the PR description that should contain a link |
There was a problem hiding this comment.
Later I think we will delegate pr descriptions to a skill but this is good.
Lets make the body of the pr follow the file https://github.com/flutter/packages/blob/main/.github/PULL_REQUEST_TEMPLATE.md. You can link it as a relative path from this directory.
3 participants
Adds a new
pre-push-skillskill that verifies changes made to a packages are ready to be "pushed", i.e. the committed changes could successfully be made into a pull request that follows the requirements of the flutter/packages repository (including some of the items in the checklist below).Pre-Review Checklist
[shared_preferences]///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Note: The Flutter team is currently trialing the use of Gemini Code Assist for GitHub. Comments from the
gemini-code-assistbot should not be taken as authoritative feedback from the Flutter team. If you find its comments useful you can update your code accordingly, but if you are unsure or disagree with the feedback, please feel free to wait for a Flutter team member's review for guidance on which automated comments should be addressed.Footnotes
Regular contributors who have demonstrated familiarity with the repository guidelines only need to comment if the PR is not auto-exempted by repo tooling. ↩ ↩2