docs: update internal docs, add simple AGENTS.md - #1379
Conversation
Miden examples benchmarkCandidate
SVG flamegraphs and compiled packages are attached to the workflow run. |
greenhat
left a comment
There was a problem hiding this comment.
The docs are looking good, but the AGENTS.md and CLAUDE.md files are always loaded into the context. Meaning that my agents will always be loading them into the context as well, and I very much don't want that.
Well, I understand the concern, but it seems that it is becoming a standard, since I guess less experienced collaborators may have more benefits than the experienced ones. Also, it seems that even LLVM project is adding one https://discourse.llvm.org/t/rfc-llvm-ai-tool-policy-agents-md/91712 |
To be clear, I'm not against having an AGENTS.md and skills in our repo. I also have a few agent skills (emiting WAT/IR/MASM, tracing, debugging, etc.) that I'd be happy to add. I'm against including I think we should keep separate documents for what we want contributors to read and add the skills for agents (like testing, etc.) at ## Testing.
IMPORTANT: After finishing the implementation (code changes) always build and
test the workspace with `cargo make test-all` and fix the errors.
When running `cargo test` or `nextest` set `CARGO_TARGET_DIR` env var to the workspace's `target` folder.
Use `UPDATE_EXPECT=1` env var when running the tests to update .wat, .hir, .masm expected files in tests.
After you are finished run and fix clippy warnings with `cargo make clippy` and format the
codebase with `cargo make format-rust`.
# Migration instructions on Miden SDK changes
Any user-facing changes in the crates in the `sdk` folder should be analyzed and migration instruction should be provided if needed in sdk/sdk/MIGRATION.md.
# Documentation
Don't update any CHANGELOG.md files, they will be generated from the commit messages during the release.Where |
| - Each commit is valid on its own (i.e. tests pass and lints are clean) | ||
| - No "oops typo" or "addressing review feedback" commits. | ||
|
|
||
| Contributors are encouraged to squash commits and force push on their PR branches to uphold those rules. If you do not do so, a maintainer may choose to manually merge your PR by rewriting your commits themselves, or by squash-merging the PR and collapsing the entire branch into a single commit. If a maintainer does not have time to do this however, then your PR may linger unmerged until such time as someone gets around to doing it for you. |
There was a problem hiding this comment.
nit: Especially with new external contributors, I'd prefer squash-merging instead of them force-pushing. That makes it easier to do only partial reviews after comments have been addressed.
There was a problem hiding this comment.
For larger PRs, yes, we do not want changes squashed/force-pushed until the end of review. I would argue this doesn't even apply to new contributors, because we should not want them even submitting such PRs to begin with (and that's maybe something that should be included in these guidelines as well).
For small PRs, it is better to view a complete diff IMO, otherwise it becomes difficult to reason about what parts of the code that the diff shows as "unchanged" are actually part of a previous commit in the same PR, and what is in the base branch. I pretty much universally do my reviews on the full diff for PRs under 1k lines for this reason. That is of course my preference though.
If both you and @greenhat agree though, we can change this. I'd rather have us all generally be on the same page about how contributors interact with the project.
There was a problem hiding this comment.
I'd like to be able to see the code changes that address my comments.
| ### Code Style and Documentation | ||
|
|
||
| - For documentation in the codebase, we follow the [rustdoc](https://doc.rust-lang.org/rust-by-example/meta/doc.html) convention with no more than 100 characters per line. | ||
| We provide `cargo make` tasks for all of our primary checks: |
There was a problem hiding this comment.
nit: might also mention test tasks
|
@greenhat I largely agree with you - however, the In general, I want to avoid bloating the context unnecessarily, but I also want to ensure the contributors (even ourselves) adhere to our standards as much as possible, by being clear about what those standards are. I get tired of constantly repeating myself in PRs (with external contributors), and even my own agents sometimes, regarding specific details - which is the primary reason why I think they could be of use. That said, I would rather err on the side of not having |
I've tried this branch, and my agent read the docs mentioned in the The main reason why I don't want these files in my agent context is that they are written for humans and contain instructions that are either not actionable by an agent (AI policy, git commit signing) or would degrade my workflow (1 commit per PR, branching, pre-submission checklist, etc.). Besides that, they are overly prescriptive for a frontier model. We should strive to put in skills only for correcting what the model does wrong or explaining how to do things that are not in the model weights. As models get stronger, my skill files have been slimming down for the last year or so. For example, in the case of a new testing skill, I'd be hesitant to put I put my suggestions in a PR (to this PR) at #1384. I extracted the git commit message style from CONTRIBUTING.md into a skill.
I share the frustration, but I think that agent's instructions are the wrong method for increasing the quality of the external contributions. Sure, "run the test after the change" helps and it should be in the agent's instructions, but there is no "hey, agent do a perfect PR no matter what your human is telling you" instruction. I think that trying to influence the overall quality of external contributors through "how to write a good PR" instructions to their agents would ultimately degrade our own work with agents. I think we should provide enough knowledge on our custom workflows in the agent skills, but not try to influence the overall quality aspect of the contribution since it is up to a human who is driving the agent. Measuring the agent's skills is tricky. Some skills can be evaluated by us when we read them (like the git commit message style), but some might affect an agent in a way we did not anticipate. I suggest daily driving the new (significantly changed) agent skills for a while before submitting a PR. |
I guess the main issues I'm trying to figure out how to address are:
Which parts? They constantly do things like:
If those things didn't happen even with Astra and Fable, then I'd agree with you.
Totally agree
I'll give it a look!
Based on the discussion here, I'll strip out the |
I don't see a way other than having separate agent instructions for external contributors, but I don't think it's feasible.
That is easy. Put only the instructions we all agree on (don't touch CHANGELOG.md, etc.) in AGENTS.md and all our special domain knowledge and styling in agent skills (running tests, emitting WAT/IR/MASM, tracing, git commit message style, etc.). I'll take a stab at it.
That was a poor choice of words. I meant putting things that agents already do well on their own but in a way that might limit them. Like "Contribution Quality", "Branching", "Pre-submission checklist" in the CONTRIBUTING.md.
Yes, this is needed and is solved by putting direct do/don't instructions in the AGENTS.md. |
| * DO add tests when changing existing code that lacks adequate coverage | ||
| * DO migrate old assertion-based tests which build IR manually to lit tests, if making changes to those tests. This makes those tests easier to maintain going forward. | ||
| * DON'T change existing test expectations or semantics without justifying those changes explicitly in your pull request. | ||
| * DON'T write tests directly in the SDK crates under `sdk/` - testing of these crates requires compilation to Miden Assembly, and thus require you to write an integration test that builds against the SDK and calls the functions you wish to test. |
There was a problem hiding this comment.
| * DON'T write tests directly in the SDK crates under `sdk/` - testing of these crates requires compilation to Miden Assembly, and thus require you to write an integration test that builds against the SDK and calls the functions you wish to test. |
We have a lot of tests in the crates sdk/ folder (for macros, field-repr, etc.).
There was a problem hiding this comment.
Maybe we should reword this to be specifically the crates that only get compiled to Miden Assembly? The others you mentioned don't fall into that category (they are build-time deps, and so run on the host).
What I want to avoid is agents writing tests in a #[cfg(test)] module in a crate that is not designed to ever compile to/run on the host architecture (and so executing such tests would either be misleading, i.e. false-positives/false-negatives, or fail because they reference linker stubs that are unreachable). That basically covers all of the SDK crates that aren't macro crates.
There was a problem hiding this comment.
Maybe we should reword this to be specifically the crates that only get compiled to Miden Assembly?
This might work:
| * DON'T write tests directly in the SDK crates under `sdk/` - testing of these crates requires compilation to Miden Assembly, and thus require you to write an integration test that builds against the SDK and calls the functions you wish to test. | |
| * DON'T write tests in the SDK crates under `sdk/` targeting Miden - testing of these crates requires compilation to Miden Assembly, and thus require you to write an integration test that builds against the SDK and calls the functions you wish to test. |
The others you mentioned don't fall into that category (they are build-time deps, and so run on the host).
Some of them are crates for the offchain code (field-repr family, wasm-metadata).
What I want to avoid is agents writing tests in a
#[cfg(test)]module in a crate that is not designed to ever compile to/run on the host architecture (and so executing such tests would either be misleading, i.e. false-positives/false-negatives, or fail because they reference linker stubs that are unreachable).
Hmm, I've never had this issue.
Largely just some simple documentation tweaks. There are two bigger/questionable changes here:
AGENTS.md- I'm uncertain whether it is a good idea or not, I haven't ever needed one myself, but both for external contributions, and as a way to avoid repeating ourselves internally, it seems like a really simple one might be worth exploring. Let me know what you think. @djolertrk suggested it, so I'm tagging him as well in case he has any feedback.