feat: Add project_context_generator and project_context_updater agent skill #3903
feat: Add project_context_generator and project_context_updater agent skill #3903aasthabharill wants to merge 4 commits into
project_context_generator and project_context_updater agent skill #3903Conversation
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a structured approach to project documentation by adding two new agent skills. These skills automate the generation and maintenance of a Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![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 introduces two new agent skills: project-context-generator and project-context-updater, along with their respective test plans, templates, and registration in the skills index. The feedback highlights several improvement opportunities, including automatically detecting the git repository root, scoping git and PR searches to the specific project directory in monorepos, avoiding absolute paths for portability, resolving the chicken-and-egg issue when referencing the current PR, and correctly handling updates to existing PRs rather than blindly calling gh pr create.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3903 +/- ##
============================================
+ Coverage 55.52% 63.41% +7.88%
+ Complexity 6578 2327 -4251
============================================
Files 1103 514 -589
Lines 67634 29969 -37665
Branches 7590 3303 -4287
============================================
- Hits 37557 19005 -18552
+ Misses 27659 10004 -17655
+ Partials 2418 960 -1458
🚀 New features to boost your workflow:
|
| # Project Context Updater - Agent E2E Test Plan | ||
|
|
||
| ## Prerequisites |
There was a problem hiding this comment.
How is TEST.md used? Is there some infra we have built around testing the skill?
There was a problem hiding this comment.
this file is used in g3 skills - its not an actual test that executes, but its a reference point for a new user trying to use the skill and check if it still works as intended. The user can manually run the test by prompting the agent to perform the tasks though
We need to write EVALs to actually run tests on a skill but think thats a g3 internal framework
There was a problem hiding this comment.
I don't think its valuable in its current form. Lets remove it.
| name: project-context-updater | ||
| description: >- | ||
| Reads a project-context.md file to quickly get context on a project's | ||
| architecture and tools. Update the project-context.md | ||
| file AFTER completing a code change to ensure the documentation remains | ||
| accurate and to add any new learnings, troubleshooting tips, or example | ||
| PRs to the AI Agent Tips section. Use this skill to quickly ramp up on a | ||
| codebase using project-context.md. Don't use when instructed to create a | ||
| project-context.md file from scratch (use | ||
| project-context-generator instead). |
There was a problem hiding this comment.
Why do we need 2 skills? Can't we have 1 skill that does all CRUD associated with project context?
Also, how do we ensure that the proposed edits say coherent over days/weeks?
My initial intuition is that these DataflowTemplates are not very complex for an agent to read and understand, do you have practical examples where having a project context helped?
My gut tells that this would be more useful across extremely complex codebases, and I want you to weigh the benefits of having this v/s the overhead to maintain a consistent, coherent copy of this file over many weeks/months.
Let me know what you think?
There was a problem hiding this comment.
-
The two skills are serving different purposes and need to be called upon in different scenarios, so don't want to merge them. The generator skill is a one-time call to create a project-context.md file, while the updater skill is to be called upon while making any siginificant change to the repository to ensure project-context.md stays relevant
-
I think the intention behind adding this project-context file is more for these purposes:
- call out and add developer gotchas and correct things that the AI gets wrong often.
- call out known limitations, and workarounds that we want code to account for
I also think knowing a bunch of our more complex functionality and usecases upfront by reading this doc can help the AI account for edge cases that might not be clearly obvious in code. So its to provide some more direction of thought to the AI. So i dont think the size of the codebase should impact this usecase.
The current doc might be a bit barebones, the team needs to take up maintaining it by calling the updater tool at the end of each meaningful session. While that might sound like effort, it will help both the AI and other team members to keep gotchas in mind while coding
I do think maybe i can see if some of the sections (like testing practices) are actually required or helpful as that's easily inferable.
Here is an example
There was a problem hiding this comment.
I have a couple of thoughts here. Let's break it down -
The generator skill is a one-time call to create a project-context.md file
I think we own a handful of templates here (4-5). Do we need this one-time skill to be committed when the probable expected usage is a total of 5 times, and then sparingly after that? (whenever we add a net new template or module maybe?)
updater skill is to be called upon while making any siginificant change to the repository to ensure project-context.md stays relevan
What/Who decides that the changes are significant enough for an update to the project context? As I said earlier, I understand the spirit of what this is trying to do, I'm just not convinced we need a dedicated set of CRUD skills around the context file.
I think the intention behind adding this project-context file is more for these purposes:
- call out and add developer gotchas and correct things that the AI gets wrong often.
- call out known limitations, and workarounds that we want code to account for
This I agree with the most. However, this is an argument supporting the usefulness of having a context file, which was never in question.
I also think knowing a bunch of our more complex functionality and usecases upfront by reading this doc can help the AI account for edge cases that might not be clearly obvious in code.
Again, agree with this. The non-obvious use-cases, gotchas, edge cases, when written in a context, should be useful to AI agents.
All of this leads us to the same conclusion - a curated, accurate CONTEXT.md is the real thing of value here. The initial focus of this exercise should be to write that context accurately first, and not build scaffolding skills around it. Once that is done, we can then think about how to maintain it.
From the previous PR, I saw a bunch of comments from Vardhan around content in the context file for bulk migration which was not accurate, which got me concerned (hence the pushback). That has a higher disruptive potential since agents may mistake it for truth and get lost.
Suggested Next Steps
- We should park adding the CRUD skills around the context files for now.
- We should focus on writing a good, validated context files for the templates that we own. We can start small (for e.g. the bulk migration context you have already committed). We should absolutely use AI to define the format, write up the content, but verify that every single fact in that context is human validated against the codebase. If that requires discussions or digging into the code for verification - we should do that.
| * **Example PRs:** | ||
| * [39a8ae5e0](https://github.com/GoogleCloudPlatform/DataflowTemplates/commit/39a8ae5e0) - Fix GCS Avro Export flow | ||
| * [90964dca6](https://github.com/GoogleCloudPlatform/DataflowTemplates/commit/90964dca6) - Add Support for UUID-based Partitioning |
There was a problem hiding this comment.
This is the kind of stuff that will get stale over weeks/months and probably confuse the agent?
There was a problem hiding this comment.
ill add a section in the updater to keep these fresh, had added one earlier but maybe got lost between commits
There was a problem hiding this comment.
How will the updater keep it fresh? How will it find the relevant new PRs or commits to link here? Do we think it will be able to do so reliably?
| # Workspace Skills Index | ||
| Auto-generated index of available skills. |
There was a problem hiding this comment.
How does this help? Who reads this? Are AI agents tuned to automatically look for this file? (Just like they look for a SKILL.md file in .agents/skills/?
There was a problem hiding this comment.
my bad, it was in the g3 skill directory structure and i assumed this was required to discover skills but that doesnt seem to be the case (for g3 or jetski). i have mainly been following existing skills and docs in g3 to write skills and usually follow the practices i see there.
I still think its worth keeping this for us to find relevant skills, but definitely not required and can be removed if you think so.
There was a problem hiding this comment.
I think its not valuable in its current form. Let's remove it.
manitgupta
left a comment
manitgupta
left a comment
There was a problem hiding this comment.
I would encourage you to please take the time to review -
- The content of the change you're raising
- The environment you're raising the PR against (DataflowTemplates) and follow its conventions.
Between this PR and the previous one there are instances of changes being made without self-verification.
- Commits containing buganizer component paths, email aliases etc.
- Containing redundant files and patterns that will actually not work on this repository.
- Incorrect phrasing (PR v/s CL) which makes it seem like the content was not self-reviewed before raising the PR.
The burden to validate (and catch) what would work and what wouldn't should not be placed on the reviewers. This is a slippery slope that will not scale.
You are expected to ensure a baseline quality of the change.
I will review the comments in a bit and respond.
2 participants
b/521646880
b/521646367
sourcedb-to-spanner,spanner-to-sourcedbanddatastream-to-spannerAI Agent TipssectionReferences: