You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "kevinjqliu (via GitHub)" <gi...@apache.org> on 2026/10/04 15:30:11 UTC
[PR] docs: add agent guidance on code comments and API surface [iceberg-rust]
kevinjqliu opened a new pull request, #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337
## Which issue does this PR close?
- N/A
## What changes are included in this PR?
Coding agents tend to be verbose: comments that restate the code, docs explaining review history, tests narrating asserts, unrelated renames, and new `pub` items nobody needs. In #3263, four follow-up commits went to just removing that churn.
- `AGENTS.md`: add guidance for coding agents. Default to no comment, keep docs to what callers need, keep diffs to the change, prefer `pub(crate)`.
- `.github/copilot-instructions.md`: have Copilot review flag comment churn, unrelated edits, and API growth instead of skipping them as trivialities.
Follows the Java repo's comment guidance ([apache/iceberg AGENTS.md comment guidance](https://redirect.github.com/apache/iceberg/pull/16998), [rewrite](https://redirect.github.com/apache/iceberg/pull/18167)).
## Are these changes tested?
Docs only.
## AI Disclosure
Implemented with GitHub Copilot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance to reduce verbosity [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#discussion_r4178337682
##########
.github/copilot-instructions.md:
##########
@@ -19,3 +19,14 @@ When reviewing pull requests, follow these rules to avoid noise and redundancy:
- First-time contributors: For users new to this repository,
explicitly instruct them to "Please check and address all review
comments in this PR."
+
+Also flag these, once per PR with the locations:
Review Comment:
note that these items that should be flagged
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance to reduce verbosity [iceberg-rust]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#discussion_r4178324077
##########
.github/copilot-instructions.md:
##########
@@ -19,3 +19,14 @@ When reviewing pull requests, follow these rules to avoid noise and redundancy:
- First-time contributors: For users new to this repository,
explicitly instruct them to "Please check and address all review
comments in this PR."
+
+Also flag these, once per PR with the locations:
+
+- Comment churn: Code comments or docs that restate the code, narrate
+ history, or justify review decisions.
+- Unrelated edits: Renames, reformatting, or reordering of code the
+ change does not otherwise need.
+- API growth: New `pub` items with no use outside the crate (see
+ `public-api.txt`). Suggest `pub(crate)`.
Review Comment:
In a library, an intended downstream API often has no in-workspace caller, so “no use outside the crate” is not evidence that `pub` is excessive. This criterion can incorrectly request `pub(crate)` for legitimate API additions; base the check on whether downstream exposure is intended or justified by the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance to reduce verbosity [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#issuecomment-5994018755
> I'd also recommend adding PR review skills to the repo so you can set expectations for reviews. Feel free to take inspiration from those in Comet
>
>https://github.com/apache/datafusion-comet/tree/main/.ai/skills
I'm wondering if we can do this, and just point Copilot at it so people can use whichever model and harness with the same markdown.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance to reduce verbosity [iceberg-rust]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#issuecomment-5986304240
I'd also recommend adding PR review skills to the repo so you can set expectations for reviews. Feel free to take inspiration from those in Comet
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance to reduce verbosity [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#issuecomment-5981938677
I like these, thanks @comphead! I’ll add them to the pr.
What are your thoughts on agent skill vs AGENTS.md? Do you think these changes are helpful in AGENTS.md?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance to reduce verbosity [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk merged PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance to reduce verbosity [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on code in PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#discussion_r4188649116
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
Review Comment:
Should this name the contract a caller must know? The Java guidance this follows lists it explicitly ("Document thrown exceptions, null behavior, and resource ownership", [AGENTS.md](https://github.com/apache/iceberg/blob/17d375a0ed3a9213935f59e04404b83c64e982a6/AGENTS.md?plain=1#L117)). The Rust equivalent is the `# Errors` and `# Panics` sections from the [Rust API guidelines](https://rust-lang.github.io/api-guidelines/documentation.html#function-docs-include-error-panic-and-safety-considerations-c-failure), and the crates already have 31 `# Errors` sections. As I read the current wording, an agent following "one-line doc summary" could drop them from new public functions.
```suggestion
- Public items get a one-line doc summary of what they do for the caller,
not how. Expand only for contract the caller must know, such as the
errors it returns (`# Errors`) or when it panics (`# Panics`).
```
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
+- Test names say what they check. Do not narrate asserts.
+- Do not add tests that duplicate existing coverage.
+- Do not rename, reformat, or reorder code unrelated to the change.
Review Comment:
Should this also cover refactors and API changes? A bug fix that carries an API redesign is the scope creep I see most often from agents, and rename, reformat, and reorder don't name it.
```suggestion
- Do not rename, reformat, refactor, or reorder code unrelated to the
change. A bug fix does not carry an API redesign.
```
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
+- Test names say what they check. Do not narrate asserts.
+- Do not add tests that duplicate existing coverage.
+- Do not rename, reformat, or reorder code unrelated to the change.
+- Reuse existing helpers and types before adding new ones.
Review Comment:
Could this name the two places agents most often miss? One is test scaffolding, where [`TableTestFixture`](https://github.com/apache/iceberg-rust/blob/df4975b930533eb0c4a2535e16e9e206e64835f1/crates/iceberg/src/test_utils/scan.rs#L77) already covers most scan setup. The other is arrow-rs, where agents tend to hand-write per-row loops that an existing kernel already does.
```suggestion
- Reuse existing helpers, types, and test fixtures (such as
`TableTestFixture`) before adding new ones. Use arrow-rs kernels
instead of reimplementing them.
```
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
+- Test names say what they check. Do not narrate asserts.
+- Do not add tests that duplicate existing coverage.
Review Comment:
Both test rules here push toward fewer tests. What do you think about adding a line that keeps edge cases and error paths in scope, so agents don't read this as "test only the happy path"? Asserting against a whole expected value (for example a full `Schema`) instead of checking fields one at a time also fits the goal of this PR, since it makes tests shorter and their failures easier to read.
```suggestion
- Test names say what they check. Do not narrate asserts.
- Cover the edge cases and error paths the change adds, not only the
happy path.
- Build the expected value and assert equality with it, rather than
checking fields one at a time.
- Do not add tests that duplicate existing coverage.
```
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
+- Test names say what they check. Do not narrate asserts.
+- Do not add tests that duplicate existing coverage.
+- Do not rename, reformat, or reorder code unrelated to the change.
+- Reuse existing helpers and types before adding new ones.
+- Use the narrowest visibility that works. Make items `pub` only when they
+ are part of the API the change intends to expose; `public-api.txt`
+ changes show API growth.
+
+## Pull requests
+
+- Follow the PR template. Keep each section short: the problem, the
+ approach, and anything a reviewer would not guess from the diff.
Review Comment:
What do you think about saying what goes in the AI Disclosure section? The [Iceberg AI contribution guidelines](https://github.com/apache/iceberg/blob/17d375a0ed3a9213935f59e04404b83c64e982a6/site/docs/contribute.md?plain=1#L67) that the template links ask authors to "call out any areas of uncertainty or assumptions ... so reviewers can focus on them." With "keep each section short" and nothing else, I'd expect agents to fill that section with a one-line tool name, which drops the part reviewers use.
```suggestion
- Follow the PR template. Keep each section short: the problem, the
approach, and anything a reviewer would not guess from the diff. In
AI Disclosure, name the areas of uncertainty or assumptions a reviewer
should focus on.
```
##########
AGENTS.md:
##########
@@ -22,6 +22,33 @@
This file provides repository-specific guidance for automated agents working
in this repository.
+## Comments, docs, and diff scope
+
+Default to no comment. Leave existing comments as they are unless you change
+the code they describe or they are wrong.
+
+- Comment only what the code cannot say: invariants, non-obvious
+ constraints, links to external behavior. Do not restate the code.
+- Do not describe how the code changed or explain review history
+ ("previously", "instead of", "unlike X"). Put that in the commit message
+ or PR description.
+- Public items get a one-line doc summary of what they do for the caller,
+ not how. Expand only for contract the caller must know.
+- Test names say what they check. Do not narrate asserts.
+- Do not add tests that duplicate existing coverage.
+- Do not rename, reformat, or reorder code unrelated to the change.
+- Reuse existing helpers and types before adding new ones.
+- Use the narrowest visibility that works. Make items `pub` only when they
+ are part of the API the change intends to expose; `public-api.txt`
+ changes show API growth.
Review Comment:
Should this tell the agent how `public-api.txt` gets updated? Each published crate has its own file, generated by [`make generate-public-api`](https://github.com/apache/iceberg-rust/blob/df4975b930533eb0c4a2535e16e9e206e64835f1/Makefile#L66-L73) and checked in CI by `make check-public-api`. Without the command, an agent may hand-edit the file or leave it stale and fail CI.
```suggestion
- Use the narrowest visibility that works. Make items `pub` only when they
are part of the API the change intends to expose. Run
`make generate-public-api` to update each crate's `public-api.txt`, and
check its diff for API growth.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance to reduce verbosity [iceberg-rust]
Posted by "comphead (via GitHub)" <gi...@apache.org>.
comphead commented on PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#issuecomment-5982256528
> What are your thoughts on agent skill vs AGENTS.md? Do you think these changes are helpful in AGENTS.md?
AGENTS.md makes more sense to me as this is project wise skill for any agent. The good followup would also be to have a kinda precommit hooks to perform code analysis and fix it. Also run tests, to avoid spending ASF CI resources on partially ready PRs like https://github.com/apache/datafusion/blob/main/AGENTS.md#before-committing
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance to reduce verbosity [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#issuecomment-5981796056
I'm still new at this coding agent / review agent thing, so not sure if this is the best. PTAL!
I asked my agent how to improve this, so it might be referential 😆 🤷
cc @blackmwk @CTTY @laskoviymishka @mbutrovich @dannycjones @comphead
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance on code comments and API surface [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on code in PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#discussion_r4178302378
##########
.github/copilot-instructions.md:
##########
Review Comment:
these instructions were copied over from iceberg-cpp, and can use some tuning
##########
AGENTS.md:
##########
Review Comment:
we only had "Security Model" in this file before
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance to reduce verbosity [iceberg-rust]
Posted by "mbutrovich (via GitHub)" <gi...@apache.org>.
mbutrovich commented on PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#issuecomment-6002594516
I'm taking a pass through this now.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] docs: add agent guidance to reduce verbosity [iceberg-rust]
Posted by "andygrove (via GitHub)" <gi...@apache.org>.
andygrove commented on PR #3337:
URL: https://github.com/apache/iceberg-rust/pull/3337#issuecomment-5986307713
> I'd also recommend adding PR review skills to the repo so you can set expectations for reviews. Feel free to take inspiration from those in Comet
https://github.com/apache/datafusion-comet/tree/main/.ai/skills
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org