andygrove opened a new pull request, #6017:
URL: https://github.com/apache/datafusion-comet/pull/6017
## Which issue does this PR close?
None; filed directly.
## Rationale for this change
The PR template does not currently mention the `review-comet-pr` skill, so
contributors using an
AI coding agent have no prompt to run it on their own work. Asking them to
do that up front should
catch some of the routine Spark compatibility issues (null handling, type
coercion, ANSI mode,
missing data types, thin test coverage) before a human reviewer spends time
on them.
The problem is that the skill could not actually be run before the PR
existed. It was built
entirely around an open PR: it calls `gh pr view`, `gh pr diff`, and `gh pr
checks`, and it reads
existing review comments so it does not duplicate feedback others already
gave. An author had no
way to point it at work in progress. The best they could do was open a draft
PR first and review
it afterwards, which is backwards from what we want.
So this adds the local mode first, and then asks about it in the template.
## What changes are included in this PR?
**`.ai/skills/review-comet-pr/SKILL.md`** now runs in one of two modes,
chosen up front:
- **PR mode**, unchanged. Pass a PR number.
- **Local mode**, new. Pass nothing, and it reviews the working tree against
`main`.
Local mode resolves the base by walking `apache/main`, `upstream/main`,
`origin/main`, `main` and
taking the first that exists, preferring the Apache remote because a fork's
`origin/main` is often
stale. It reviews committed, staged, and unstaged changes together, and
lists untracked files
separately, since those do not show up in `git diff` at all and a brand-new
serde file or SQL test
file is frequently the most important file in an expression change.
The CI step becomes a local verification step in that mode. Rather than
reading `gh pr checks`, it
runs the targeted checks the author can still act on, and it calls out the
two gaps a local run
leaves behind:
- A local build does not cover the Spark profile matrix, so a change that
compiles under the
default profile can still break another one.
- A new test suite has to be named in both `pr_build_linux.yml` and
`pr_build_macos.yml`, which
`dev/ci/check-suites.py` enforces, or the suite silently never runs.
Steps 2 through 5, 7, and 8 are mode-independent and untouched. The skill
still only reports
findings, and now says explicitly that it must not edit the code in local
mode either, so the
author stays in control of what changes.
**`.github/pull_request_template.md`** gains a section asking whether the
author ran the skill. It
documents both invocations, pre-PR first, points agents that do not
auto-load skills at the
`SKILL.md` path, and offers an explicit opt-out for contributors who do not
use an agent. It is
worded as a request rather than a requirement.
One thing I noticed while working on this, not fixed here: CI's prettier
step globs `**/*.md`,
which does not descend into the dot-directory `.ai/`, so nothing under
`.ai/skills/` is
format-checked. I left the skill's existing formatting alone rather than
reflowing the whole file
in this PR. Happy to fix the glob separately if we want those files covered.
## How are these changes tested?
No automated tests; this is documentation and agent tooling.
I ran the new local mode on this change as an end-to-end check of the mode
itself. Base resolution
picked `apache/main` correctly, and the diff, uncommitted, and untracked
commands each returned
what they should at the point I ran them. The review steps that carry the
weight for expression
work (Spark source comparison, serde, Rust, benchmarks) are all not
applicable to a markdown-only
change, so this exercised the plumbing rather than the analysis.
`prettier --check` passes on the PR template. Worth noting that `prettier
--check "**/*.md"`
currently flags two files on `main` that this PR does not touch,
`docs/source/contributor-guide/debugging.md` and
`docs/source/contributor-guide/native_shuffle.md`,
most likely version drift since CI installs the latest prettier.
## Have you run the Comet review skill on this PR?
- [x] Yes, I ran `review-comet-pr` and addressed the findings
- [ ] No, I do not use an AI coding agent
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
