[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, the mechanical checks run on the added lines only: vague "native", config keys, operator names, links and anchors, images, external links, structure, version claims, and TODO markers. The commands use git grep -P and perl, because macOS grep has no -P and git grep -E silently ignores \b.
  • 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.

[sunchao]

[P2] gh pr view --json files can silently truncate the list used to build <paths>. On a large PR, documentation after the first 100 files can escape review entirely. Enumerate the changed paths from the fetched merge base and head with git diff --name-only -z.

Evidence: With gh 2.98.0, Comet PR #5206 reported changedFiles=139 and returned 100 files. The paginated files API placed CometCoverageStatsSuite.scala at position 139. Its added class Scaladoc is in scope. PR #5206

[sunchao]

[P2] Check that git merge-base succeeded before collecting added lines. A shallow checkout can fetch both tips and verify the correct head while still lacking their common ancestor. Deepen the history or report the comparison incomplete, and propagate failures from git diff.

Evidence: In a depth-one fixture, both refs fetched and the head matched. git merge-base exited 1, then the documented diff pipeline printed fatal: bad revision '', exited 0, and emitted zero lines. After deepening, it emitted the edit at docs/source/page.md:3.

[sunchao]

[P2] Save the verified head commit ID for subsequent reads. HEAD_REF currently names a ref that another fetch of the same PR can advance. Even with private scratch files, an earlier review can then quote new source against its old line list.

Evidence: In a two-fetch fixture, the first head passed the equality check and its private list recorded page.md:2: old wording. After fetching the updated PR, git show through that same ref returned new wording. Reading the originally verified SHA still returned old wording.

[sunchao]

[P2] Give each invocation its own state file and carry that path into later shell calls. Two different PR reviews in one checkout currently overwrite review-comet-docs.env, so the first review can silently load the second PR’s base, head, and added lines.

Evidence: I interleaved the documented saves for PR 1 and PR 2 in a disposable checkout. The first review’s next shell successfully loaded refs/review/pr-2, base-2, and PR 2’s scratch path.

[sunchao]

[P2] Serialize the saved values so paths containing spaces round-trip through a fresh shell. In a linked worktree, git rev-parse --git-dir can return an absolute path. The current printf writes that path as an unquoted assignment, preventing later checks from opening ADDED.

Evidence: In a linked worktree with its main checkout under repo with space, saving succeeded. Sourcing the resulting state file returned 127 and left ADDED empty.

[sunchao]

[P2] Check incoming references when a PR removes or renames a heading or explicit label. The added-line list contains the new heading, while the link that the change breaks can be on an unchanged page. Search for references to the old target or validate the affected documents with Sphinx.

Evidence: A two-page fixture initially passed sphinx-build -W -n. Renaming ## Install to ## Setup made the only added line +## Setup. Sphinx then identified the unchanged index.md:3 link to howto.md#install as myst.xref_missing.

[sunchao]

[P2] Include untracked indexes in the audit-mode toctree search. An author can create a section index and child page before staging either. The audit inventories both files, but this command misses the valid navigation entry and can report it as must-fix.

Evidence: An untracked topic/index.md listed child inside its toctree. The documented git grep returned exit 1. Adding --untracked found the entry at topic/index.md:4.

[sunchao]

[P2] Resolve a toctree entry relative to the document that contains it before counting it as a match. A basename such as index can match a valid entry for a different section, allowing a newly orphaned page to pass the navigation check.

Evidence: After adding second/index.md without a navigation entry, the prescribed search succeeded on first/index inside the root toctree. The base Sphinx build passed. The head build reported that second/index.md was not included in any toctree.

[sunchao]

[P2] Use a format-aware heading check for .rst files. The selector includes reStructuredText, but the heading command recognizes only lines beginning with Markdown # markers. It cannot validate the title or section hierarchy of an RST page.

Evidence: The documented command exited successfully with zero headings for docs/source/user-guide/latest/index.rst. Docutils parsed lines 21–23 of that same file as the title Comet $COMET_VERSION User Guide.

[sunchao]

[P2] Track the opening Markdown fence’s character and length, or read parsed heading nodes. Toggling the fence state on every triple delimiter treats a shorter fence inside a code example as its end and can report a shell comment as a second H1.

Evidence: In a page with one H1 and a four-backtick example containing a triple-backtick shell snippet, the command reported H1s at lines 1 and 5. The CommonMark parser returned only the heading at line 1.