-
Notifications
You must be signed in to change notification settings - Fork 3.8k
Adds pre-push readiness skill #11935
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 15 commits
5b96272
7b56f17
5d7c78a
21ab658
68d3fa5
aa81a3f
241121b
437dced
23ae1ac
7f41a50
ca8d5a1
c56dadf
2d38f9b
f72fcf0
7cc1c56
3698672
0d967b4
e641398
d4bd26a
b3cb4a4
b2d4321
c9cdb4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Meta comments:
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -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." | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure this should trigger on request for "code review before committing". Consider instead
Suggested change
Why did you need "flutter/flutter#188373"? This skill should be loaded locally so it should not leak to those environments.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Accepted minus the notes on engine/framework |
||||||
| --- | ||||||
|
|
||||||
| # Pre-Push Skill | ||||||
|
|
||||||
| This skill provides a fully automated pre-push validation script | ||||||
| and a mental checklist for developers attempting to push code changes | ||||||
| to any of the packages in the flutter/packages repository. | ||||||
| It directly answers the question: **"Am I ready to push?"** | ||||||
|
|
||||||
| Follow the steps below before pushing any current code changes upstream | ||||||
| via `git push` to a package or finishing your task. | ||||||
| If any step fails, continue with the following steps | ||||||
| to provide a comprehensive list of required fixes for the user. | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ## 1. Initial Clean Working Tree Check | ||||||
|
|
||||||
| Because the tooling natively relies on `git diff` | ||||||
| to determine which packages have changed, | ||||||
| it **completely ignores uncommitted changes**. | ||||||
| You must ensure the working tree is clean before running any checks. | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
| Command to run: | ||||||
|
|
||||||
| ```bash | ||||||
| git status --porcelain | ||||||
| ``` | ||||||
|
|
||||||
| If this command outputs anything, the code WAS NOT ready to push. | ||||||
| The developer must commit or stash their changes | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
| before you can proceed with the remaining validation steps. | ||||||
| Do not continue if there are uncommitted changes. | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| ## 2. Check for Changed Packages | ||||||
|
|
||||||
| Because `--run-on-changed-packages` defaults to checking the entire repository | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
| if zero packages have changed, you must verify | ||||||
| that there are actually package changes to test. | ||||||
| Command to run: | ||||||
|
|
||||||
| ```bash | ||||||
| git diff --name-only main...HEAD | grep '^packages/' | ||||||
| ``` | ||||||
|
|
||||||
| If this command outputs nothing, then no packages were modified in this branch. | ||||||
| You can skip all remaining validation steps | ||||||
| and proceed directly to the Final Review. | ||||||
| If it outputs file paths, continue to the next step. | ||||||
|
|
||||||
| ## 3. Format Code | ||||||
|
|
||||||
| Consistent code style is required for all pull requests. | ||||||
| The repository uses auto-formatters (like `dart format`, `clang-format`) | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not not but maybe later we should reference the check-readiness skill to ensure the formatters are on path and acceptable. |
||||||
| to enforce this. Command to run: | ||||||
|
|
||||||
| ```bash | ||||||
| dart run script/tool/bin/flutter_plugin_tools.dart \ | ||||||
| format --run-on-changed-packages | ||||||
| ``` | ||||||
|
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just saw that we could also use the
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think what you have is good already!
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
|
|
||||||
| If this command modifies any files, the code WAS NOT ready to push. | ||||||
| Those changes must be committed before the developer can push. | ||||||
|
|
||||||
| ## 4. Static Analysis | ||||||
|
|
||||||
| Static analysis catches potential bugs, type errors, | ||||||
| and style violations without needing to run the code. | ||||||
| All code must pass the analyzer without any warnings or errors. | ||||||
| Command to run: | ||||||
|
|
||||||
| ```bash | ||||||
| dart run script/tool/bin/flutter_plugin_tools.dart \ | ||||||
| analyze --run-on-changed-packages | ||||||
| ``` | ||||||
|
|
||||||
| If this command fails, the code WAS NOT ready to push. | ||||||
| Those analyzer errors must be fixed and committed before the developer can push. | ||||||
|
|
||||||
| ## 5. Unit Tests | ||||||
|
|
||||||
| Tests ensure that your changes do not break existing functionality | ||||||
| and that new features work as expected. | ||||||
| All unit tests must pass before code can be merged. | ||||||
| Command to run: | ||||||
|
|
||||||
| ```bash | ||||||
| dart run script/tool/bin/flutter_plugin_tools.dart \ | ||||||
| dart-test --run-on-changed-packages | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again this is a place where we could simplify the command if we scoped it to camera_android_camerax
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
| ``` | ||||||
|
|
||||||
| If this command fails, the code WAS NOT ready to push. | ||||||
| Those test errors must be fixed and committed before the developer can push. | ||||||
|
|
||||||
| ## 6. Publish Check (Version and CHANGELOG updates) | ||||||
|
|
||||||
| Any pull request that changes non-test code | ||||||
| must increment the package version in `pubspec.yaml` | ||||||
| and add a corresponding entry describing the change in `CHANGELOG.md`. | ||||||
| Command to run: | ||||||
|
|
||||||
| ```bash | ||||||
| dart run script/tool/bin/flutter_plugin_tools.dart \ | ||||||
| publish-check --run-on-changed-packages | ||||||
| ``` | ||||||
|
|
||||||
| If this command fails, the code WAS NOT ready to push. | ||||||
| The required version bump and changelog entry must be made | ||||||
| and committed before the developer can push. | ||||||
| This can be done automatically by running: | ||||||
|
|
||||||
| ```bash | ||||||
| dart run script/tool/bin/flutter_plugin_tools.dart update-release-info \ | ||||||
| --version=minimal --base-branch=main \ | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
| --changelog="<description of your changes>" | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
| ``` | ||||||
|
|
||||||
| ## 7. License Headers | ||||||
|
|
||||||
| All source files in this repository must include | ||||||
| the standard copyright and license header. | ||||||
| Command to run: | ||||||
|
|
||||||
| ```bash | ||||||
| dart run script/tool/bin/flutter_plugin_tools.dart license-check | ||||||
| ``` | ||||||
|
|
||||||
| If this command fails, the code WAS NOT ready to push. | ||||||
| Those license errors must be fixed and committed before the developer can push. | ||||||
|
|
||||||
| ## 8. Final Clean Working Tree Check | ||||||
|
|
||||||
| Before pushing, ensure that all your fixes, formatting changes, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||||||
| and version bumps are committed. Command to run: | ||||||
|
|
||||||
| ```bash | ||||||
| git status --porcelain | ||||||
| ``` | ||||||
|
|
||||||
| If this command outputs anything, the code WAS NOT ready to push. | ||||||
| The changes must be cleaned up before the developer can push. | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| --- | ||||||
|
|
||||||
| ## Final Review | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| Before declaring the task complete, | ||||||
| verify the final requirements for creating a pull request | ||||||
| in the flutter/packages repository. | ||||||
|
|
||||||
| **What you MUST verify automatically:** | ||||||
| - **Documentation:** Check if the modified or newly added public APIs | ||||||
| include Dart doc comments (`///`). If any are missing, proactively fix them | ||||||
| or ask the user if they'd like you to add them. | ||||||
| - **Tests:** Check if any test files were added or modified | ||||||
| alongside the source code changes. If not, proactively ask the user | ||||||
| if they'd like you to generate tests (Critical for PR acceptance!). | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| **What you MUST NOT hold against the developer:** | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Did this happen during your work? I would be surprised if it did.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Deleted. It didn't happen to me, just wanted to explicitly cover the checklist and be clear about what to fail on versus not. But you're right, this is out of scope! |
||||||
| Do NOT penalize or block the developer for administrative checklist items | ||||||
| that are unverifiable by an agent | ||||||
| (e.g., signing the CLA, reading the Contributor Guide). | ||||||
|
|
||||||
| **Action to take:** | ||||||
| First, explicitly state the final verdict to the user | ||||||
| at the very beginning of your response using a large heading: | ||||||
|
|
||||||
| - If ANY step failed: Start your response with a clear | ||||||
| "# NO, you are not ready to push." | ||||||
| followed by a summary of what failed and what needs to be fixed. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I really like that you gave the explicit format. |
||||||
| - If ALL steps passed: Start your response with a clear | ||||||
| "# YES, you are ready to push!" | ||||||
|
|
||||||
| Then, provide the developer with a brief summary | ||||||
| of what you verified automatically. | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
| If the code is ready to push, provide them with this short checklist | ||||||
| of items they will need to handle when opening their pull request: | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
|
|
||||||
| - Ensure the PR title starts with the package name in brackets | ||||||
| (e.g., `[camera_android] Fix crash`). | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
| - Ensure the PR description links to at least one issue that is being fixed. | ||||||
| - Ensure they have signed the CLA. | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
| - Ensure the branch is up to date with the main branch | ||||||
|
camsim99 marked this conversation as resolved.
Outdated
|
||||||
| and has no merge conflicts. | ||||||
Uh oh!
There was an error while loading. Please reload this page.