Update agent tooling - #10202
Update agent tooling#10202cwfitzgerald wants to merge 14 commits into
Conversation
110c19f to
aff08ef
Compare
|
Imo, you should also add the Vulkan/DirectX/Metal/Spirv/etc specs. I have been asking claude to check its code/design against the specs a lot. |
|
That sounds good, but can be a follow up, there's already a bunch going on in this PR |
03f46e2 to
53f13e5
Compare
ErichDonGubler
left a comment
There was a problem hiding this comment.
Not committing to a full review (but open to it). Some thoughts I had while skimming.
b0180d7 to
5a386c7
Compare
|
Does |
|
Let me test it, my understanding is that this is a thing that the agent has to explicitly take action on and do a tool call but I could be wrong. |
5a386c7 to
416f504
Compare
416f504 to
830977b
Compare
ErichDonGubler
left a comment
There was a problem hiding this comment.
RE: changes requested: This LGTM overall, but I've noticed a number of things that give me pause with immediately merging. I've tried to be explicit about what I consider blocking and what's not, but may have missed something; LMK if anything's unclear, I'm out of time to review on this.
meta: This…was a lot to review. The commits in here were sensible, and following them wasn't difficult (❤️). To recap my understanding, this PR:
- Adds an
ai-commit-checkjob in CI - Adds
cargo xtask sync-metadatawith another new CI job, which enforces:- Sync'ed skill contents for different LLM tools
- Sync'ed software license files between all crates
I'm not sure I agree that it made sense to keep all of these as a single PR.
- I can see some defense for just wanting the unit of review for all the pieces of
sync-metadatato be commits, rather than a PR (minus how complicated FS code is, which I definitely didn't have time to review fully). - However, the AI commit check specifically seems entirely orthogonal.
I used Conventional Comments in this review! I hope they help with clarity and tone. 🙂
| Run `sh .agents/skills/webgpu-specs/download.sh` to download the | ||
| WebGPU and WGSL specifications if they are not present or if they have | ||
| been updated. You do not need to change directory before running the script. | ||
|
|
||
| After the specs are downloaded, you can search in `target/claude/webgpu-spec.bs` | ||
| and `target/claude/wgsl-spec.bs` for relevant sections of the specification. | ||
|
|
There was a problem hiding this comment.
nit(non-blocking): I suspect we'll need to include some criteria for invalidating cached copies of the spec., since we're an audience that is sensitive to recent changes.
There was a problem hiding this comment.
There's a "if they have been updated" in the previous sentence, we'll see if that's enough. It may or may not be.
|
|
||
| For the WGSL specification, the base URL is <https://gpuweb.github.io/gpuweb/wgsl/>. | ||
|
|
||
| If necessary, read additional content from the file to find the header preceding |
There was a problem hiding this comment.
thought: Do we need to be more specific about using links? We don't provide concrete examples or even Web-centered terminology in what we're expecting here. In order of priority, I'd hope the agent tries to provide:
- A link to the header via anchor (i.e., navigating to the
id="section"part of the page with#section) - A link to a text fragment (i.e.,
#:~:text=Some%20specific%20text)
These can be combined (i.e., #section:~:text=Some%20specific%20text) so that (2) is attempted, but if the text isn't found, then it falls back to (1). This seems like a very helpful combination with graceful behavior if the text changes enough to break a text fragment link.
There was a problem hiding this comment.
Potentially, though the agent should have some idea how bikeshed works. I'll add this as a potential follow up.
| ## Squash or Rebase? | ||
|
|
||
| _If your pull request contains multiple commits, please indicate whether | ||
| they need to be squashed into a single commit before they're merged, | ||
| or if they're ready to rebase onto `trunk` as they stand. In the | ||
| latter case, please ensure that each commit passes all CI tests, so | ||
| that we can continue to bisect along `trunk` to isolate bugs._ | ||
|
|
There was a problem hiding this comment.
thought: After many months of growing to dislike filling out this section of the template, I suspect it would be better if this were enforced with CI or even not at all.
We already have selection of a merge method via a label with Mergify now. Maybe we could use a check to block CI for such a label if all the commits aren't a base plus optional fixup!/FIXUP:/etc. commits?
There was a problem hiding this comment.
CC @gfx-rs/wgpu here; I'll raise this in today's maintainers' meeting. ☝🏻
| @@ -1,34 +1,45 @@ | |||
| **Connections** | |||
| ## Connections | |||
There was a problem hiding this comment.
praise: I've long wondered why we didn't use section headers. This feels welcome. ❤️
| // In `Check` mode errors on any differences, in `Write` mode | ||
| // deletes extra files/directories, copies updated ones, and makes | ||
| // missing ones. | ||
| fn sync_directory( |
There was a problem hiding this comment.
thought: We're taking on quite a bit of scope here with file synchronization. That's unfortunate, but I'm not sure I see a better way than what's already written (yet).
File synchronization is a notoriously complicated problem, usually with notoriously complicated solutions. This has real tension with wanting to keep xtask fast to compile (viz., simple code). I suspect we're going to be fixing bugs in our own code here for a while, but they'll only as they slowly (and intermittently) show up. I'm concerned about that, but not in a way that makes me want to block.
I don't see anything on Crates.io that looks like a fit with (1) being fast to compile, (2) has the features/behavior we need (particularly with diagnostics), and (3) is trustworthy. If you're interested, we can talk about what exists out there as a potential replacement in an issue.
There was a problem hiding this comment.
What gottchas are you thinking of - this doesn't need to be a generic implementation of syncing, it's very well bounded:
- No symlinks
- One file system
- Copies are one direction, with one side having authority (so no diffs)
This is basically glorified rm + cp.
| // Both sides of the `strip_prefix` below must be canonical. Windows | ||
| // canonicalization adds a `\\?\` prefix that `cargo metadata` does not use. |
There was a problem hiding this comment.
nit: We should use dunce to implement canonicalization, if this is a concern.
There was a problem hiding this comment.
I'm not sure what the actual difference here for us - dunce has the advantage of keeping the paths human readable, but NT UNC paths are just as canonical
|
|
||
| ## Repository Metadata | ||
|
|
||
| `AGENTS.md` and `.agents/` are the source of truth for agent instructions and skills. `CLAUDE.md` and `.claude/` are generated copies. Do not edit the generated copies directly. After you edit any source-of-truth metadata file or a root license file, run `cargo xtask sync-metadata`. This command also copies the root license files to every publishable default member of the Cargo workspace. |
There was a problem hiding this comment.
suggestion(non-blocking): As follow-up, it'd be nice to have these disclaimers in the .claude files themselves.
There was a problem hiding this comment.
I don't want to take up additional LLM context with this, but we could add a README in the .claude which says don't edit these files. LLMs won't as they have this context, but a human might, but will be caught by CI.
|
This is an amazingly thorough and thoughtful review, thank you!! |
stuartparmenter
left a comment
There was a problem hiding this comment.
I suspect you could simplify some of the skills a bit for some frontier models, but them being more verbose is probably better for other models -- I generally wouldn't overthink them though as you'll end up just chasing model behavior
Connections
Closes #10055
Description
Please review this commit-by-commit, these are standalone commits. A lot of this PR is adding duplicate files.
This reworks our agent tooling:
sync-metadataand CI job to check that this stays up to date.Testing
Checked that new claude sessions have the CLAUDE.md context, checked that Codex has the webgpu-spec skill. Tests exist for the sync-metadata xtask
Squash or Rebase?
Rebase, each commit is standalone.
Checklist
wgpumay be affected behaviorally.CHANGELOG.mdentries for the user-facing effects of this change are present.