Skip to content

Conversation

@eugene-manuilov
Copy link
Collaborator

Summary

Addresses issue:

Relevant technical choices

PR Author Checklist

  • My code is tested and passes existing unit tests.
  • My code has an appropriate set of unit tests which all pass.
  • My code is backward-compatible with WordPress 5.2 and PHP 7.4.
  • My code follows the WordPress coding standards.
  • My code has proper inline documentation.
  • I have added a QA Brief on the issue linked above.
  • I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

  • Run the code.
  • Ensure the acceptance criteria are satisfied.
  • Reassess the implementation with the IB.
  • Ensure no unrelated changes are included.
  • Ensure CI checks pass.
  • Check Storybook where applicable.
  • Ensure there is a QA Brief.
  • Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

  • Ensure the PR has the correct target branch.
  • Double-check that the PR is okay to be merged.
  • Ensure the corresponding issue has a ZenHub release assigned.
  • Add a changelog message to the issue.

@eugene-manuilov eugene-manuilov marked this pull request as ready for review January 4, 2026 08:31
Copy link
Collaborator

@aaemnnosttv aaemnnosttv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @eugene-manuilov, this is a good start, just a few things to address and consider.

Comment on lines +75 to +80
- name: Run /implement command
run: |
gemini --yolo --model gemini-3-pro-preview "/implement ${{ inputs.issue_number }}"
env:
GEMINI_API_KEY: ${{ secrets.GEMINI_API_KEY }}
GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we can't further restrict the capabilities of the GITHUB_TOKEN here, we should perhaps exclude it since the model should only need read-only access which it should already have as a public repo. If it is needed, perhaps we can provide a PAT with read-only access. Pushing to the repo doesn't happen until the next step anyways so I'd think this should be ok.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, we need it here. I have updated the gemini settings to restrict the GitHub MCP to allow only the issue toolchain and excluded all tools except the one that reads issue details. Will it work for you?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks – can you be more specific as to what the token is needed for? If it's readonly, then I would think it might even work without a token at all (except for rate limits), but otherwise, we could provide a token which is readonly using a fine-grained token. Even with the limited toolchain, I don't think we should expose/provide a model which is capable of writing anything.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, let's use a fine-grained token that has access only to public repositories. Do you want me creating it or you will create it and add as GITHUB_READONLY_TOKEN?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can do that, but it would require adding another token that needs rotating due to policy limitations.

How about we split it into two jobs and configure the permissions for each to be scoped as needed? That way we can still use the default GITHUB_TOKEN but it only needs write access to create the PR (correct?). I realize this would make it a little slower because we'd need to upload the changes as an artifact and then download them again afterwards to create a PR with.

Alternatively, we could keep it as a single job and change the permissions to be read only and then use the bot token for creating the PR since permissions only apply to the github token? This might be more appropriate anyways. WDYT?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I have moved PR creation and comment posting into a new job with write permissions and updated this job to have read-only permissions. It should solve all our concerns here, right?

Copy link
Collaborator

@aaemnnosttv aaemnnosttv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @eugene-manuilov , this is nearly there. Just a few last details. See also my reply about the token permissions.

Comment on lines 42 to 51
"exclude": [
"github__add_issue_comment",
"github__assign_copilot_to_issue",
"github__get_label",
"github__issue_write",
"github__list_issue_types",
"github__list_issues",
"github__search_issues",
"github__sub_issue_write"
]
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we define this as an allowlist instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, good idea, updated.

Comment on lines 107 to 111
**1. Understand the Task**
- Carefully read the Implementation Brief from Phase 1
- Break down complex requirements into discrete steps
- Plan your implementation approach before writing code
- Identify all files that need to be created, modified, or deleted
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's update this to mention the AC being the actual requirements that must be met now that it's included in the context collected in phase 1.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Follow these implementation guidelines:
**1. Understand the Task**
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might not make a difference for the model to comprehend, but as an ordered list, I think the formatting should only apply to the list item text, not the number, otherwise it doesn't become an actual list.

e.g.

  1. step 1
  2. step 2

rather than

1. step 1
2. step 2


Looking below, I can see the formatting is applied inconsistently where both forms are used in different places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

Comment on lines +233 to +234
Check compliance with the extracted "Acceptance criteria":
- ✓ Implementation fulfills all points in "Acceptance criteria"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great! I still think it's probably worth mentioning in the instructions to understand the task beforehand though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where you mean we need to add it. Can you clarify?

implement:
name: Implement Issue
runs-on: ubuntu-latest
timeout-minutes: 60
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems longer than reasonable. How long have we seen it take so far?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know how much time it can take when we actually trigger it as a GitHub workflow. Let's have 60 for now and update once we see how it actually works.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants