Skip to content

chore(repo): add docs reviewer checks to review-pr skill - #36562

Merged
FrozenPandaz merged 3 commits into
masterfrom
NXC-4728
Aug 5, 2026
Merged

chore(repo): add docs reviewer checks to review-pr skill#36562
FrozenPandaz merged 3 commits into
masterfrom
NXC-4728

Conversation

@jaysoo

@jaysoo jaysoo commented Aug 4, 2026

Copy link
Copy Markdown
Member

Current Behavior

The review-pr skill has no docs-specific checks. PRs touching astro-docs content get only a thin editorial-direction note at trim time.

Expected Behavior

A new read-only docs-reviewer agent runs when the diff touches astro-docs content and checks the changed pages against astro-docs/STYLE_GUIDE.md and the CLAUDE.md docs instructions, plus structural hazards (redirects for moved pages, sidebar-label-coupled routes, Markdoc validity). Its verdicts feed the review verdict like the other analyzers.

Example

I used /review-pr skill on #36508.

Here's what was returned in the review when I asked:

/btw was docs-reviewer agent used, why or why not. if so, what was surfaced?

    Yes, docs-reviewer was dispatched — as part of the full 10-agent fleet, not conditionally.

    Why it ran: the skill treats docs-reviewer as an always-run agent, on two grounds. Its coverage question ("does this change leave prose docs stale or missing?") applies to every diff, since a code change can invalidate prose without touching a docs file. And its compliance question applies here
    specifically because the diff does change docs content — 3 of the 10 changed files are under astro-docs/src/content/docs/ (kb/resolve-circular-dependencies.mdoc, reference/nx-json.mdoc, reference/environment-variables.mdoc). So both halves of its remit were live.

    Verdict: DOCS_CONCERN, EVIDENCE line 81, verified.

    What it surfaced:

    1. The finding that mattered most — it corrected my charter. I had asserted in pre-established measurement 6 that no redirect covered /docs/troubleshooting/resolve-circular-dependencies, making the new {% linkcard %} href a reader-facing 404. That was wrong: netlify.toml lives at
    astro-docs/netlify.toml, not the repo root, and my pre-dispatch grep targeted the wrong path. Lines 951-952 there already 301 that exact path to /docs/kb/resolve-circular-dependencies, pre-existing on base, well ahead of the /docs/* catch-all at 1117. I confirmed this directly afterward.

    1. It then kept the finding alive on a different, narrower basis: validate-links.ts compares rendered hrefs against the built sitemap with no redirect awareness, and astro-docs:test lists validate-links in dependsOn — so it breaks CI rather than readers. It explicitly rated this important rather
    than DOCS_BROKEN for that reason, which was the right call. code-reviewer independently reached the same correction and conclusion.
    2. A style-guide violation Vale can't catch — the rewritten NX_IGNORE_CYCLES cell uses a semicolon; STYLE_GUIDE.md says use two sentences. No Vale rule covers semicolons, so it would have shipped.
    3. Coverage came back clean, with the sweep shown — it checked kb/enforce-module-boundaries.mdoc, kb/incremental-builds.mdoc and concepts/* for prose the new flag might have staled, and found none, correctly reasoning that an off-by-default option changes no existing page's described behavior.
    4. Structural checks clean — no pages added/moved/renamed/deleted, so no redirects or sidebar.mts changes owed; Markdoc valid ({% cardgrid %}/{% linkcard %} nesting matches siblings, both new fences are ```jsonc with the // nx.json first-line comment rather than a title= attribute).
    5. Suggestions — cross-page duplication (the KB page restates the reference section near-verbatim, same code block included), the "derived command graph / task graph" point repeated three times in twelve lines, the new anchor having no inbound link, and the new config flag being positioned above
    the actual remediation steps on a page whose job is fixing cycles.

    One boundary worth noting: editorial direction (whether docs recommend something the team shouldn't encourage) is deliberately not this agent's call — that stays with the orchestrator at trim time. Nothing in this diff triggered it.

Related Issue(s)

Fixes NXC-4728


Polygraph View session ↗

@netlify

netlify Bot commented Aug 4, 2026

Copy link
Copy Markdown

Deploy Preview for nx-dev ready!

Name Link
🔨 Latest commit ba0f4cc
🔍 Latest deploy log https://app.netlify.com/projects/nx-dev/deploys/6a737614074f4200083d7e3f
😎 Deploy Preview https://deploy-preview-36562--nx-dev.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@netlify

netlify Bot commented Aug 4, 2026

Copy link
Copy Markdown

Deploy Preview for nx-docs ready!

Name Link
🔨 Latest commit ba0f4cc
🔍 Latest deploy log https://app.netlify.com/projects/nx-docs/deploys/6a737614c51de90008ecbe1c
😎 Deploy Preview https://deploy-preview-36562--nx-docs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@nx-cloud

nx-cloud Bot commented Aug 4, 2026

Copy link
Copy Markdown
Contributor

View your CI Pipeline Execution ↗ for commit ba0f4cc

Command Status Duration Result
nx run-many -t check-imports check-lock-files c... ✅ Succeeded 3s View ↗
nx affected --targets=lint,test,build,e2e,e2e-c... ✅ Succeeded <1s View ↗
nx-cloud record -- pnpm nx-cloud conformance:check ✅ Succeeded 1m 1s View ↗
nx build workspace-plugin ✅ Succeeded <1s View ↗
nx-cloud record -- nx sync:check ✅ Succeeded 19s View ↗
nx-cloud record -- nx format:check ✅ Succeeded 5s View ↗

☁️ Nx Cloud last updated this comment at 2026-08-05 17:47:58 UTC

jaysoo added 2 commits August 4, 2026 16:13
review-pr has no docs-specific checks. Docs PRs get only a thin editorial-direction note at trim time.

New read-only docs-reviewer agent runs when the diff touches astro-docs content (astro-docs/src/content/ or sidebar.mts). Checks changed pages against astro-docs/STYLE_GUIDE.md and the CLAUDE.md docs instructions, plus structural hazards: redirects for moved/renamed/deleted pages (astro.config.mjs + netlify.toml), sidebar-label-coupled routes, Markdoc validity. Verdicts feed Step 7 like the other analyzers (DOCS_BROKEN critical, DOCS_CONCERN important, DOCS_SOUND to Strengths). Pipeline version bumped to 6 so stale drafts age out.

Fixes NXC-4728
## Current Behavior
Skill named nx-docs-style-check.

## Expected Behavior
Renamed check-docs-style: verb-first, nx prefix redundant in-repo. CLAUDE.md reference updated.

## Related Issue(s)
NXC-4728
@jaysoo
jaysoo marked this pull request as ready for review August 5, 2026 17:33
@jaysoo
jaysoo requested a review from a team as a code owner August 5, 2026 17:33
@jaysoo
jaysoo requested a review from lourw August 5, 2026 17:33
docs-reviewer only ran on docs diffs, so it could not flag code changes that need docs updates (the NXC-4728 goal). check-docs-style frontmatter still said nx-docs-style-check (edit was left unstaged in the rename commit). Rename detection used a three-dot base range that has no merge base in the shallow sandbox. Harmful guidance was Important in calibration but Critical in DOCS_BROKEN.

docs-reviewer always runs: coverage check on every diff (user-facing surface swept against prose docs; DOCS_UPDATE_NEEDED important; auto-generated plugin/CLI reference exempt) plus the existing compliance checks on docs diffs. Rename detection compares endpoints: git diff --name-status --find-renames origin/<BASE_REF> HEAD. Editorial direction (incl. harmful guidance) stays with the caller at trim time; DOCS_BROKEN no longer lists it. check-docs-style frontmatter name matches its directory.

NXC-4728
@FrozenPandaz
FrozenPandaz merged commit fe017cd into master Aug 5, 2026
25 checks passed
@FrozenPandaz
FrozenPandaz deleted the NXC-4728 branch August 5, 2026 18:00
polygraph-snapshot-app Bot pushed a commit that referenced this pull request Aug 11, 2026
## Current Behavior

The review-pr skill has no docs-specific checks. PRs touching astro-docs
content get only a thin editorial-direction note at trim time.

## Expected Behavior

A new read-only docs-reviewer agent runs when the diff touches
astro-docs content and checks the changed pages against
astro-docs/STYLE_GUIDE.md and the CLAUDE.md docs instructions, plus
structural hazards (redirects for moved pages, sidebar-label-coupled
routes, Markdoc validity). Its verdicts feed the review verdict like the
other analyzers.


## Example

I used `/review-pr` skill on #36508.

Here's what was returned in the review when I asked:

```
/btw was docs-reviewer agent used, why or why not. if so, what was surfaced?

    Yes, docs-reviewer was dispatched — as part of the full 10-agent fleet, not conditionally.

    Why it ran: the skill treats docs-reviewer as an always-run agent, on two grounds. Its coverage question ("does this change leave prose docs stale or missing?") applies to every diff, since a code change can invalidate prose without touching a docs file. And its compliance question applies here
    specifically because the diff does change docs content — 3 of the 10 changed files are under astro-docs/src/content/docs/ (kb/resolve-circular-dependencies.mdoc, reference/nx-json.mdoc, reference/environment-variables.mdoc). So both halves of its remit were live.

    Verdict: DOCS_CONCERN, EVIDENCE line 81, verified.

    What it surfaced:

    1. The finding that mattered most — it corrected my charter. I had asserted in pre-established measurement 6 that no redirect covered /docs/troubleshooting/resolve-circular-dependencies, making the new {% linkcard %} href a reader-facing 404. That was wrong: netlify.toml lives at
    astro-docs/netlify.toml, not the repo root, and my pre-dispatch grep targeted the wrong path. Lines 951-952 there already 301 that exact path to /docs/kb/resolve-circular-dependencies, pre-existing on base, well ahead of the /docs/* catch-all at 1117. I confirmed this directly afterward.

    1. It then kept the finding alive on a different, narrower basis: validate-links.ts compares rendered hrefs against the built sitemap with no redirect awareness, and astro-docs:test lists validate-links in dependsOn — so it breaks CI rather than readers. It explicitly rated this important rather
    than DOCS_BROKEN for that reason, which was the right call. code-reviewer independently reached the same correction and conclusion.
    2. A style-guide violation Vale can't catch — the rewritten NX_IGNORE_CYCLES cell uses a semicolon; STYLE_GUIDE.md says use two sentences. No Vale rule covers semicolons, so it would have shipped.
    3. Coverage came back clean, with the sweep shown — it checked kb/enforce-module-boundaries.mdoc, kb/incremental-builds.mdoc and concepts/* for prose the new flag might have staled, and found none, correctly reasoning that an off-by-default option changes no existing page's described behavior.
    4. Structural checks clean — no pages added/moved/renamed/deleted, so no redirects or sidebar.mts changes owed; Markdoc valid ({% cardgrid %}/{% linkcard %} nesting matches siblings, both new fences are ```jsonc with the // nx.json first-line comment rather than a title= attribute).
    5. Suggestions — cross-page duplication (the KB page restates the reference section near-verbatim, same code block included), the "derived command graph / task graph" point repeated three times in twelve lines, the new anchor having no inbound link, and the new config flag being positioned above
    the actual remediation steps on a page whose job is fixing cycles.

    One boundary worth noting: editorial direction (whether docs recommend something the team shouldn't encourage) is deliberately not this agent's call — that stays with the orchestrator at trim time. Nothing in this diff triggered it.
```

## Related Issue(s)

Fixes NXC-4728

<!-- polygraph-session-start -->
---
<p><picture><source media="(prefers-color-scheme: dark)"
srcset="https://static.ops.cloud.nx.app/polygraph/session-logo-v4-dark.svg"><img
src="https://static.ops.cloud.nx.app/polygraph/session-logo-v4-light.svg"
width="16" height="22" align="middle" alt="Polygraph"></picture> <a
href="https://snapshot.app.trypolygraph.com/orgs/69cdc268b6aa527e4129c2b4/sessions/humble-beaver-3540ffdd">View
session ↗</a></p>
<!-- polygraph-session-end -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants