[Abhishek-Mohan]

Which issue does this PR close?

Closes #4420.

Rationale for this change

#4420 asks for a docs-focused sibling of review-comet-pr that checks documentation against the #4419 style guide and against the code, and gives advice to a reviewer rather than editing or commenting.

This PR is stacked on #7151, which adds the style guide the skill reads. The first commit here is #7151's style guide commit. I'll rebase once #7151 merges.

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 real base branch, and fetches both into review refs without touching the working tree.
  • Branch mode (<branch>): reviews a branch, tag, or commit before it becomes a PR.
  • Audit mode (<path>): reviews an existing file or directory as it is now.

• What it reviews: Markdown under docs/source, README.md, changed .doc(...) strings, and changed doc comments (Scaladoc, Javadoc, Rust ///).

• 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.
  • In PR and branch mode, it pins the reviewed commit, lists changed files with git diff, and runs the line checks on the added lines only: vague "native", config keys, operator names, external links, and version claims. The commands use git grep -E with perl, so they work without a PCRE build of Git and on macOS.
  • Links, anchors, images, toctrees, and page structure are checked by building the docs for the base and the head with Sphinx and comparing the warnings.
  • Then it runs judgment checks. The most important is whether each claim still matches the code.

• Output: findings as must-fix, should-fix, or nit, each with path:line, rule ID, quote, and suggested rewrite. It also reports the revisions it reviewed and any guide gaps. It never edits files, commits, pushes, or posts.

• Checking This Skill: a short table of fixed test cases with their expected results, to rerun after changing the skill or the guide.

• review-comet-pr changes:

  • a routing row for docs/source/, README.md, and .doc(...) strings;
  • documentation added to its description;
  • "docs only" removed from the list of PRs it covers on its own.

  The routing table is also edited by docs: add a scan contributor guide and a scan PR review skill #6052, so I'll rebase this row after whichever lands first.

Severity tiers. #4420 asked for must-fix, should-fix, and nit. review-comet-pr later added "There is no third tier" (#5057). The skill keeps all three tiers for audits and for authors reviewing their own work. It 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:

1. 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.
2. Check generated docs? No. The skill checks the sources they're generated from, and a hand edit to generated content is a must-fix (P5).
3. Pin the guide's path or read the latest? Both: the path is fixed, and the skill reads the file on every run. In PR mode it reads the PR's version if the PR changes the guide.
4. A separate audit skill? No. One skill with three modes keeps the checks in one place.

How are these changes tested?

After the test runs below, five independent AI reviews (GPT-6 Astra, Grok 4.7, Gemini 4 Argon, Claude Opus 5.5, GPT-6.1 Sol) ran the skill's commands on real pages and PRs. After two rounds, these findings are fixed:

• the config-key search reads definitions that span two lines, including contrib keys and createExecEnabledConfig names, and leaves out deprecated alternative names. It finds 136 keys on this branch.
• PR mode uses the PR's real base and the + refspec, and checks that the head it fetched is the PR's current head;
• the operator check lists only concrete classes, including those in the per-Spark-version source directories;
• the toctree check matches every page, including the root index.md;
• the heading check skips code blocks;
• generated content is limited to the text between the generator markers, and review-comet-pr and review-comet-expression-pr now point to the guide for that;
• the wording exemption for code no longer turns off the accuracy checks;
• there is a new check that moved or deleted pages have redirects (P17).

I ran every command in the current skill on this Mac against the #7151 branch.

Using the skill on its own PRs. I ran it with five models (Claude Opus 5.5, GPT-6.1 Sol, GPT-6 Astra, Grok 4.7, Gemini 4 Argon), each in a separate throwaway clone. Each model ran it twice: in PR mode on #7151, and on itself (PR mode on this PR, plus an audit of its own SKILL.md).

• On docs: add a documentation style guide and apply it to the remaining pages #7151, it found real errors, which are now fixed there:
  • a seventh explain config the page leaves out;
  • conditions missing from the native columnar-to-row and direct-read sentences;
  • the wrong sources in the Generated Content table.
• On itself, it found bugs in its own commands and instructions, which are now fixed:
  • \bnative\b misses Markdown italics such as _native execution_;
  • the diff parser broke on file names with spaces and on added lines starting with ++;
  • the heading check missed ~~~ and indented code fences;
  • audit mode ignored files that aren't committed yet;
  • the anchor rule didn't match MyST, which keeps _ in slugs;
  • the key list included an unexpanded template;
  • two expected results in its own test table were wrong.
• In review-comet-pr, it found two problems that were already on main:
  • gh pr checks --failed no longer exists;
  • spark.comet.expr.allowIncompatible isn't a real key.

Earlier, I ran the skill with subagents on real pages and PRs, fixed what the runs exposed, and re-ran two of them.

Run	Expected	Result
Audit plugin_overview.md	Flag the removed Parquet reader (lines 89-92) and "natively" (lines 113, 117)	All flagged as must-fix, plus a wrong description of rule registration. That's fixed in #7151.
Audit understanding-comet-plans.md	No false positives on "native shuffle", CometNativeScan, or config keys	None. It also found real problems: missing operators, "five configs" when six are listed, and stale sample output.
Audit timezones.md (contributor guide)	Allow "native code", "native plan", and similar	Round 1 flagged "native kernel" and similar, so N3 was widened in #7151. Round 2: 5 findings out of 30 hits, all real.
PR mode on #6786	Catch the drift in datasources.md	Two must-fix errors: the paragraph and .doc string claim more than the code does, and "unlike the switches above" contradicts line 57.
PR mode on #7009	Don't "fix" Iceberg wording that the PR just matched to the code	No must-fix. It checked about 15 claims against the code and found a stale code comment the docs now contradict.

The test runs also changed the skill:

• Portability. Use git grep -P everywhere. macOS grep has no -P, and git grep -E silently ignores \b.
• PR mode. Commands now search the PR ref, count only hits on changed lines, and also read the PR's code diff.
• Guide. Added P16 (describe the code as it is now) so that false or out-of-date claims have a rule to cite.

On these pages, a round-2 run took 10-15 tool calls.

Sample output: PR mode on #6786 (round 2, shortened)

Scope. PR mode, refs/review/pr-6786. Reviewed datasources.md:77-86 and the new .doc(...) for spark.comet.convert.broadcastBuildSide.enabled, plus the code diff in CometExecRule.scala.

Check	Hits	Findings
Native / vectorized	3 on changed lines	folded into must-fix 1 and 2
Config keys	14 named	0 missing
Structure	31 headings	0
Version / TODO	0 on changed lines	0

Must-fix

1. datasources.md:77-80, P16 (also N1, N3): "a file or V2 scan Comet cannot read natively … the whole join runs natively". shouldBridgeUnsupportedScan is reached only from the fallback branch, so CSV, JSON, and Parquet scans are not covered. The conversion also needs the broadcast exchange and join configs, and clearWastedBuildSideBridges can undo it. Suggested: "…a scan of a file format other than Parquet, JSON or CSV … so the join can run in the Comet pipeline."
2. CometConf.scala:1225-1228, P16: the same overclaim in the .doc string, which is published in configs.md.
3. datasources.md:81, P16: "on by default, unlike the switches above". spark.comet.convert.oneRowRelation.enabled is also on by default, and line 58 says so. Suggested: "which is on by default."

Already on the page: datasources.md:62 "not generate natively" (N1); :63 "Comet's native cache scan" (N3).

Sample output: audit of timezones.md (round 2, shortened)

Must-fix

1. timezones.md:115, P13: lists ToJson and ToCsv, but the expression classes are StructsToJson and StructsToCsv.
2. timezones.md:144-145, P16 and N1: "date_format runs natively only in UTC sessions". CometDateFormat also takes the Rust path in non-UTC sessions when allowIncompatible is set.

Should-fix: four N1 uses (lines 198, 219, 235, 258), and P8 on line 187 ("Spark 3.x", "Spark 4.0+").

Nit: mention spark.comet.testing.strictTimestampLabels.enabled in the testing section.

Guide gaps: heading case is not covered.

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

[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.

[Abhishek-Mohan]

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.