docs: add a docs-review skill by Abhishek-Mohan · Pull Request #7153 · apache/datafusion-comet

Abhishek-Mohan · 2026-10-22T00:14:32Z

Which issue does this PR close?

Closes #4420.

Rationale for this change

#4420 asks for a docs-focused sibling of review-comet-pr: a skill that checks documentation against the #4419 style guide and against the code, and advises a reviewer rather than editing files or posting comments. The style guide merged in #7151, so this PR now changes only the skill files.

What changes are included in this PR?

New .ai/skills/review-comet-docs/SKILL.md, with three modes:
- PR mode (<pr-number>): reviews the docs a PR changes. It finds the Apache remote and the PR's base branch, fetches both into review refs without touching the working tree, and pins the head it reviews.
- Branch mode (<branch>): reviews a branch, tag, or commit before it becomes a PR.
- Audit mode (<path>): reviews an existing page or directory as it is now, including files not committed yet. This is the mode for the page sweep in docs: move existing pages to the style guide's "native" rules #7358.
What it reviews: Markdown and reStructuredText under docs/source, README.md, SVG text under docs/source, AGENTS.md, skills, changed .doc(...) strings, and changed doc comments. In code files it checks only doc comments and .doc(...) strings, and it skips test sources.
How it works:
- It reads style_guide.md on every run and cites rule IDs (T, N, O, P) instead of restating the rules. If the guide and the skill disagree, the guide wins. The guide applies to new and changed text, so in PR and branch mode the skill reports existing problems separately, as "already on the page".
- Mechanical checks run on the added lines in PR and branch mode, or on the whole files in audit mode: vague "native" and "vectorized", config keys against the keys the code defines, operator names against the operator classes, version claims, and external links. The commands use git grep -E with perl, so they work on macOS and without a PCRE build of Git, and they handle file names with spaces or non-ASCII characters.
- Docs build: links, anchors, images, toctrees, and page structure are checked by building the base and the head with Sphinx and comparing their warnings. Each revision builds with its own docs/requirements.txt, in a virtual environment it creates with Python 3.10 or later. A failed build is reported as not checked, never as clean. Links to headings that GenerateDocs adds to the per-version expression pages are checked against the serde maps instead, since this build copies only the templates.
- Judgment checks: most importantly, whether each claim still matches the code at the reviewed head.
- Each step says what to do when it can't run, and the skill keeps its state in a file so an agent can follow it across separate shell calls in bash or zsh.
Output: findings as must-fix, should-fix, or nit, each with path:line, rule ID, the quoted text, and a suggested rewrite, plus the revisions reviewed, a table of the mechanical checks, and any gaps in the guide. It never edits files, commits, pushes, or posts.
Checking This Skill: a table of fixed test cases and their expected results, to rerun after changing the skill or the guide.
review-comet-pr and review-comet-expression-pr:
- review-comet-pr routes docs, README.md, AGENTS.md, skills, .doc(...) strings, and doc comments to this skill, mentions documentation in its description, and no longer handles docs-only PRs on its own.
- Both point to the guide's Generated Content section for what not to hand-edit.
- The routing table is also edited by docs: add a scan contributor guide and a scan PR review skill #6052. If that lands first, I'll merge main and resolve the row.

Severity tiers. #4420 asked for must-fix, should-fix, and nit. review-comet-pr later settled on two tiers (#5057). This skill keeps all three for audits and for authors checking their own work, and drops nits when it runs as part of a review-comet-pr review, so that skill's review bar still decides what goes on a PR.

Answers to #4420's open questions:

Stricter rules for the user guide? Yes. N3 allows "native code" and similar only in the contributor guide, and P14 requires a runnable example only on new user-guide pages.
Check generated docs? No. The skill checks the sources they're generated from, and a hand edit to generated content is a finding (P5).
Pin the guide's path or read the latest? Both: the path is fixed, and the skill reads the file on every run, from the revision under review.
A separate audit skill? No. One skill with three modes keeps the checks in one place.

How are these changes tested?

This PR adds instructions for an agent, not code, so it has no unit tests. It was tested by running it:

Dry runs by seven models. Claude Opus 5.5, Claude Sonnet 5.5, Gemini 4 Argon, Grok 4.7, GPT-6.1 Sol, GPT-6 Astra, and GPT-6 Luna each followed the skill literally in zsh on macOS, in their own clones. Each ran a real branch-mode review of docs: add a documentation style guide and apply it to the remaining pages #7151 and one case from the skill's own test table. Every gap they hit was checked against the code and fixed, over four rounds (scores below). Their review of docs: add a documentation style guide and apply it to the remaining pages #7151 also found real errors there, which were fixed before it merged.
The test table. Each row passed in the later rounds, after the fixes:
- PR mode on feat: run broadcast joins natively when an unsupported scan is on the build side #6786, which predates the guide: it reports that datasources.md and the new .doc string claim more than shouldBridgeUnsupportedScan does.
- Audits of understanding-comet-plans.md, timezones.md, and the expression compatibility directory: no false positives on the allowed terms, protobuf names, or generated sections.
- A scratch page with banned phrases: the expected N1 and N5 findings.
Shell commands. I ran each command in bash and zsh. That includes a page whose name has a space and an accent, an empty file list, a build with only Python 3.9 available, and a head that pins a different Sphinx version.
Review fixes. The findings from comphead's and sunchao's reviews are fixed. The shell bugs among them (file names with spaces and accents, the shared docs environment) were reproduced first and then rechecked.
Docs build. Building main and this branch with Sphinx gives no new warnings, and prettier passes.

Council review

Each round, seven models ran dry runs: they followed the skill literally as an agent would, in branch, PR, and audit mode, including the cases in its own test table. Models already at 10/10 were not rerun in round 4, and the one gap Opus found in round 4 is fixed.

Rounds: 4

Delegate	Round 1	Round 2	Round 3	Round 4
Claude Opus 5.5	7/10	8/10	8/10	9/10
Gemini 4 Argon	8/10	10/10	10/10	not run
Grok 4.7	6/10	7/10	9/10	10/10
GPT-6.1 Sol	8/10	10/10	10/10	not run
GPT-6 Astra	7/10	10/10	9/10	10/10
Claude Sonnet 5.5	6/10	7/10	7/10	9/10
GPT-6 Luna	8/10	7/10	8/10	10/10

AI assistance: OpenCode helped write the skill and ran the test reviews. The findings above came from those runs.

Add docs/source/contributor-guide/style_guide.md, which writes down the vocabulary from apache#4419 (pipeline membership, implementation language and data format), the rule for "native", how to name operators in prose, and the page conventions that the docs-review skill in apache#4420 cites. Each rule has a stable ID so that reviews can cite it. The guide follows the code where apache#4419 and the code differ: "Hybrid" keeps the meaning it already has in expressions.md, expressions are described separately from plan nodes, and "Arrow-native" is a claim about data format only. The Generated Content section names the generated text exactly, by the markers GenerateDocs fills in, including the config tables built from QueryPlanSerde and contrib config classes, and the cast tables built from CometCast. P18 covers attribution of third-party content, which apache#4420 lists among its checks. The guide's scope covers .doc strings in contrib config classes, and its vocabulary and accuracy rules also apply to doc comments, AGENTS.md and agent skills. Link the page from the contributor guide toctree and from AGENTS.md. Part of apache#4419. Generated-by: OpenCode

Add .ai/skills/review-comet-docs/SKILL.md, a sibling of review-comet-pr for documentation. It has three modes: PR mode reviews the docs a pull request changes, branch mode reviews a branch before it becomes a PR, and audit mode reviews an existing file or directory, including files not yet committed. The skill reads the documentation style guide on every run and cites its rule IDs rather than restating the rules. It finds the Apache remote and the PR's real base, reads everything from the reviewed revision, checks whether main has moved since, and checks only the added lines in PR and branch mode. Mechanical checks come first: vague "native" (including in Markdown italics), config keys (including keys built from a name at run time), operator names, links, anchors, images, external links, structure, redirects for moved pages, and version claims. Then it runs judgment checks, the most important being whether each claim, and each condition in it, still matches the code. Besides docs pages, it checks changed .doc strings, doc comments, AGENTS.md and agent skills. Findings are must-fix, should-fix, or nit, with nits dropped when the skill runs as part of a review-comet-pr review. It never edits files or posts comments. A short table of fixed cases says how to check the skill after changing it. In review-comet-pr, route docs changes to the new skill, mention documentation in its description, and remove "docs only" from the list of PRs it covers on its own. Fix two things in it that no longer match: `gh pr checks --failed` no longer exists, and the opt-in key is spark.comet.expression.<Name>.allowIncompatible. In review-comet-pr and review-comet-expression-pr, point at the style guide for exactly which docs text is generated. Closes apache#4420. Generated-by: OpenCode

sunchao

Summary

Prior state and problem
Comet’s general review skill covered documentation freshness but gave reviewers no shared terminology or procedure for checking documentation against the source.
Design approach
The stacked prerequisite publishes a style guide with stable rule IDs. The second commit adds one documentation-review skill for PRs, branches, and working-tree audits.
Correctness
The diff extractor mapped all 559 additions in this PR to the correct head lines. Separate fixtures showed where it can omit a file, mix revisions, miss broken navigation, or report a valid page as broken.
Compatibility analysis
Spark’s TreeNode.nodeName strips Exec in all five pinned versions: 3.4.3, 3.5.9, 4.0.4, 4.1.3, and 4.2.0. Comet’s window, exchange, and take-ordered overrides support the guide’s examples. The JVM shuffle writers also confirm its description of buffering UnsafeRow before Arrow IPC encoding. Spark source
Key design decisions
The guide distinguishes pipeline membership, implementation language, and data format. Its distinction between generated tables and hand-written prose matches GenerateDocs and docs/build.sh.
Implementation sketch
The skill fetches review refs, computes a merge base, extracts added lines with git diff -U0 and awk, then compares names and claims with the reviewed source.
Performance
With PCRE-enabled Git, the config and operator inventories each took about 0.15 seconds on this checkout. The skill caps judgment checks at ten pages per run. No introduced P1/P2 execution-performance issue was found.
Design
A shared rulebook gives the three modes consistent criteria. The parent router omits two document types that the guide and child skill explicitly cover.
Abstraction & complexity
A review needs a stable base, head, and file list. Mutable refs and a shared shell-state file make those inputs depend on other invocations. The hand-written heading and navigation checks also lose information that the document parser provides.
Behavioral changes worth calling out
Relative to branch-1.2, contributors gain a published guide and a documentation-review workflow. The parent skill also corrects the per-expression allowIncompatible key and the failed-check filter. The changes affect contributor tooling and documentation.
Suggested improvements
Pin each invocation to verified commit IDs, enumerate its files through Git, and use the docs build’s target and heading rules. The inline reproductions provide focused regression cases for each failure.

Reviewed both commits and all six files at f7644b8f04f75bc207d8bf8056b129e200e56e53 against base 2b1c6a3938cbe1c4b2d0aa6383e64360569f877a, using merge base 5a50bd6f2777f3f20cefd929552d93dc6ab263c5. Routed skills were review-comet-pr, review-comet-docs, review-comet-expression-pr, and review-comet-shuffle-pr. All four specialist report digests matched. The PR was open and non-draft, with no existing reviews, conversation comments, inline comments, or review threads.

On the reviewed SHA, Label pull requests passed. Comet CI, CodeQL, and Check PR Title each concluded action_required with zero jobs. Comet CI run

Local validation covered git diff --check, disposable Git fixtures, tagged Spark source, and focused Sphinx 7.4.7/MyST 3.0.1 builds. The style guide rendered and all ten local link targets resolved. The full generated site, Spark/native suites, Prettier, and RAT were not run. Network policy blocked the external ASF licensing link check.

…phinx - Pin the reviewed commit (HEAD_SHA) after checking it against headRefOid, and read everything at it, including the style guide. - List changed files with git diff instead of gh pr view --json files, which stops at 100 files. - Fail on a missing merge base (shallow clones) instead of diffing against an empty revision. - Check drift against the PR's own target branch, not always main. - Name the state file per run and quote its values, so concurrent reviews and paths with spaces work. - Use git grep -E with perl instead of git grep -P, so Git builds without PCRE work, and set core.quotePath=false for non-ASCII names. - Replace the hand-written link, anchor, toctree, and heading checks with a Sphinx build of the base and the head, compared by warning. It resolves the generated per-version pages the way docs/build.sh does, catches links that a renamed heading breaks on unchanged pages, handles reStructuredText, and resolves toctree entries properly. H1s are counted with markdown-it, which handles nested code fences. - Route AGENTS.md and agent skills to the skill from review-comet-pr. Part of apache#4420. Generated-by: OpenCode

Abhishek-Mohan · 2026-10-28T17:34:04Z

Note

🤖 Claude Opus 5.5 responding on behalf of @Abhishek-Mohan

@sunchao all 16 findings are fixed in 6ef1f41, and main is merged in (450d37d). The link, anchor, toctree, and heading checks now use a Sphinx build of base and head. Ready for another look.

comphead

Citing the style guide's rule IDs instead of restating the rules keeps the skill and the guide from drifting, and every path, class and config the skill names exists at this head. Two comments on the shell steps and one on a self-check.

…ame, keep the N5 test case stable The merge-base step now tries again after deepening and stops with an error if there is still no base. Branch names with a slash no longer break the state file path. The N5 case moves to the scratch page, so it holds before and after apache#7151 rewrites understanding-comet-plans.md. Generated-by: OpenCode

…dit mode build() now reports a failed Sphinx run instead of comparing empty warning lists, builds the working tree in audit mode, and uses a named virtual environment, as does the H1 check. Audit mode gets a config-key command, the stop rule no longer treats an empty search as an error, the generated-content check says what to compare, and the apache#6786 case reads the guide from main. Generated-by: OpenCode

Dry runs on Claude, GPT, Gemini and Grok models in zsh on macOS found these gaps. The venv now picks Python 3.10 or later and cleans up on failure, since the macOS system Python cannot install the docs requirements. Each call reloads the state with a given line, and a failed docs build no longer reads as a reason to stop. The guide falls back to the target when the head predates it. Audit mode reads the working tree for the H1 check and code reads, and skips the rename check. Code files are filtered to doc comments, SVG text is reviewed, protobuf message names are not operators, and the build output no longer carries the macOS /private prefix. Generated-by: OpenCode

N1 now covers every form of the banned phrases, such as ran natively and Comet-native. N2 allows the native shuffle path without pairing it with JVM shuffle. N3 replaces and similar with a test, names user-guide/, and says pages outside the contributor guide allow only the N2 compounds. Agents running the guide could not apply the old wording consistently. Generated-by: OpenCode (cherry picked from commit 80d4af0)

T7 lists Arrow-native, which N3's limit to N2 compounds outside the contributor guide would otherwise forbid. N3 also says any other use of native is a finding, so a phrase outside the lists has an outcome. Generated-by: OpenCode (cherry picked from commit ad78e9b)

…ast resort The changed files that step 3 selects are now written to a file saved in the state, so later calls pass them to Git instead of rebuilding the list by hand. When neither the head nor the target has the guide, as for a PR made before it existed, the skill reads the working tree's copy and says so, instead of stopping. Audit mode leaves the revision out of the code searches, because an empty quoted revision makes Git fail. Generated-by: OpenCode

…e fallback Audit mode has no ADDED list, so the 10-file cap now counts the files step 1 lists. Test sources no longer enter the file list, since their string literals passed the doc-comment filter. The stop rule names the step 2 guide lookup as a fallback, and the key notes cover the EXPRNAME placeholder pages use. Generated-by: OpenCode

A dry run read native cache scan as allowed by N2, but the in-memory cache scan runs on the JVM. N2 now says the scan and writer compounds apply only to Rust-implemented ones. Generated-by: OpenCode (cherry picked from commit e8b8000)

A dry run rewrote "falls back to its native execution" and "executes natively (not falling back to Spark)" wrongly, because N1 offered only T1 or T3. N1 now says not falling back is T1, since codegen dispatch is not a fallback, and that Spark's own behavior is N5. Generated-by: OpenCode (cherry picked from commit 147f3f4)

Audit mode used an undefined <files> placeholder. It now saves the step 1 list as PATHS, as the other modes do, and says the mechanical checks cover every file while only the judgment checks are capped. Generated-by: OpenCode

Existing pages move over as they are edited, so a review does not ask a change to restyle text it leaves alone, unless the change makes that text wrong (P16). Requested in review. Generated-by: OpenCode (cherry picked from commit fdff0b1)

sunchao

The revised instructions pin the PR head and compare Sphinx warnings on both revisions. I rendered both docs trees and saw no additional warnings on this head. Two cases can still make the proposed audit miss a problem.

…requirements The file list was passed as $(cat "$PATHS"), which splits a name such as "My Page.md" into two pathspecs, so the page was silently skipped. Commands now pass it through tr and xargs -0 -r. The docs build reused one environment installed from the checkout's requirements; a new venv function installs each revision's own docs/requirements.txt into an environment named after its hash, so a change that updates a Sphinx extension is built with it. Requested in review. Generated-by: OpenCode

sunchao

Reviewed 3c764f1 across five independent scopes. The per-revision Sphinx environments address the prior requirements mismatch, and the changed-path pipeline preserves spaces. Two reproducible issues remain in audit file enumeration and generated-anchor validation.

Audit mode listed files with Git's default path quoting, so a page such as résumé.md became an escaped string and was skipped. The docs build copies only the expression templates, so a link to a heading that GenerateDocs adds to a per-version page warns although the published page has it; the skill now says to check the serde's notes instead. Requested in review. Generated-by: OpenCode

The audit searches now pass core.quotePath=false too, so a finding on a page such as résumé.md names the file as written. Generated-by: OpenCode

sunchao

Reviewed 678f0f6 across five independent scopes. The Unicode filename handling is fixed, and generated expression anchors are now called out explicitly. One narrower gap remains: the manual exception can clear a warning for a heading that exists on a different generated page.

A heading GenerateDocs adds appears only on the category page whose serde map holds the expression, at the Spark versions that register it. The manual check now confirms both, so a link to the same heading on another category page is reported as broken. Requested in review. Generated-by: OpenCode

Abhishek-Mohan · 2026-11-04T11:40:18Z

Note

🤖 Claude Opus 5.5 responding on behalf of @Abhishek-Mohan

@andygrove @sunchao ready for another look. With #7151 merged, this PR now changes only the skill files. All review threads are resolved, the description is updated, and CI passes on the fork.

sunchao

Reviewed 3f31e33 across five independent scopes. The manual generated-anchor check now verifies the linked category, Spark-version registration/shim, serde notes and emitted class-name heading, which addresses the prior cross-category/profile false positive. The focused documentation change and current-head fork checks look good; no remaining findings.

andygrove · 2026-11-05T21:55:31Z

Looks like this has been through a few rounds of review. Let's merge and keep iterating. Thanks @Abhishek-Mohan and thanks @comphead and @sunchao for reviews

github-actions Bot added the documentation Improvements or additions to documentation label Oct 22, 2026

This was referenced Oct 23, 2026

Establish nomenclature style guide and audit operator names for clarity #4419

Open

docs: add a documentation style guide and apply it to the remaining pages #7151

Merged

Abhishek-Mohan force-pushed the docs/4420-docs-review-skill branch 6 times, most recently from 2dc00b1 to bfc0d2f Compare October 24, 2026 00:35

Abhishek-Mohan added 2 commits October 23, 2026 18:00

Abhishek-Mohan force-pushed the docs/4420-docs-review-skill branch from bfc0d2f to f7644b8 Compare October 24, 2026 01:03

Abhishek-Mohan marked this pull request as ready for review October 24, 2026 01:46

sunchao reviewed Oct 24, 2026

View reviewed changes

Abhishek-Mohan added 2 commits October 26, 2026 21:28

Merge branch 'main' into docs/4420-docs-review-skill

450d37d

Merge branch 'main' into docs/4420-docs-review-skill

f414774

Abhishek-Mohan requested a review from sunchao October 28, 2026 22:43

comphead reviewed Oct 28, 2026

View reviewed changes

Comment thread .ai/skills/review-comet-docs/SKILL.md Resolved

Comment thread .ai/skills/review-comet-docs/SKILL.md Resolved

Comment thread .ai/skills/review-comet-docs/SKILL.md Resolved

Abhishek-Mohan added 10 commits October 28, 2026 17:56

docs: save the audit file list as PATHS

6f2fbb1

Audit mode used an undefined <files> placeholder. It now saves the step 1 list as PATHS, as the other modes do, and says the mechanical checks cover every file while only the judgment checks are capped. Generated-by: OpenCode

Abhishek-Mohan requested a review from comphead October 29, 2026 16:48

Abhishek-Mohan added 2 commits October 30, 2026 16:19

Merge branch 'main' into docs/4420-docs-review-skill

4127b42

Abhishek-Mohan mentioned this pull request Oct 30, 2026

docs: move existing pages to the style guide's "native" rules #7358

Open

48 tasks

sunchao reviewed Oct 31, 2026

View reviewed changes

Comment thread .ai/skills/review-comet-docs/SKILL.md Resolved

Comment thread .ai/skills/review-comet-docs/SKILL.md Resolved

Abhishek-Mohan added 2 commits November 1, 2026 10:18

Merge branch 'main' into docs/4420-docs-review-skill

3c764f1

sunchao reviewed Nov 2, 2026

View reviewed changes

Comment thread .ai/skills/review-comet-docs/SKILL.md Resolved

Comment thread .ai/skills/review-comet-docs/SKILL.md Resolved

Abhishek-Mohan added 3 commits November 2, 2026 18:00

docs: print audit search results with unquoted file names

dea5294

The audit searches now pass core.quotePath=false too, so a finding on a page such as résumé.md names the file as written. Generated-by: OpenCode

Merge branch 'main' into docs/4420-docs-review-skill

678f0f6

sunchao reviewed Nov 3, 2026

View reviewed changes

Comment thread .ai/skills/review-comet-docs/SKILL.md Resolved

Abhishek-Mohan added 2 commits November 4, 2026 03:28

Merge branch 'main' into docs/4420-docs-review-skill

b1fb411

Merge branch 'main' into docs/4420-docs-review-skill

3f31e33

sunchao approved these changes Nov 5, 2026

View reviewed changes

andygrove added this pull request to the merge queue Nov 5, 2026

Conversation

Abhishek-Mohan commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

How are these changes tested?

Council review

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Summary

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Abhishek-Mohan commented Oct 28, 2026

Uh oh!

comphead left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Abhishek-Mohan commented Nov 4, 2026

Uh oh!

sunchao left a comment

Choose a reason for hiding this comment

Uh oh!

andygrove commented Nov 5, 2026

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Abhishek-Mohan commented Oct 22, 2026 •

edited

Loading