feat: add CI guard requiring sidebar updates when adding docs#3089
feat: add CI guard requiring sidebar updates when adding docs#3089
Conversation
Add script/check-docs-sidebar that detects newly added published docs under docs/oss/ and docs/pro/ and verifies each has a corresponding entry in docs/sidebars.ts. Supports an explicit exclusion list at docs/.sidebar-exclusions for intentional omissions (legacy redirects, pages linked from other docs instead of the sidebar). Wire the check into: - GitHub Actions via .github/workflows/check-docs-sidebar.yml - Local CI via bin/ci-local (runs even for docs-only changes) Update AGENTS.md Boundaries/Always section with the new rule. Closes #2889 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds a docs-sidebar guard: a script that detects newly added/renamed published docs under Changes
Sequence Diagram(s)sequenceDiagram
participant Dev as Developer
participant GH as GitHub Actions
participant Repo as Repository
participant CI as Local CI (bin/ci-local)
participant Check as script/check-docs-sidebar
participant Sidebars as docs/sidebars.ts
participant Exclusions as docs/.sidebar-exclusions
Dev->>Repo: push / open PR (docs/*)
GH->>Repo: fetch base ref if needed
GH->>Check: run job with base/current refs
CI->>Check: (local) invoke script with refs
Check->>Repo: git diff → list added/renamed docs
Check->>Exclusions: load excluded IDs
Check->>Sidebars: check each doc ID presence
Sidebars-->>Check: present / missing
Check-->>GH: exit 0 if all present, exit 1 with missing IDs
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Review: CI Guard for Docs SidebarGood addition — the guard closes a real gap where newly added docs could miss sidebar registration undetected. The structure is clean, the exclusion mechanism is well-documented, and wiring it into both GHA and IssuesStaged file detection gap (most impactful)
Grep-based sidebar detection matches commented-out IDs
Positive notes
|
Greptile SummaryThis PR adds a CI guard ( Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/quality suggestions that do not affect correctness. The implementation correctly handles all documented edge cases (all-zeros SHA, untracked local files, remote fetch, three-dot diff semantics, early-exit doc-only paths). The three P2 comments are about a theoretical eval injection, a potential false-positive in the sidebar grep, and the path-filtered (vs. mandatory) nature of the workflow — none block merge. script/check-docs-sidebar (sidebar presence grep could be tightened)
|
| Filename | Overview |
|---|---|
| script/check-docs-sidebar | New bash script that detects newly added docs/oss and docs/pro files and checks they appear in sidebars.ts; logic is sound but the sidebar presence check uses a plain substring match that could produce false positives. |
| .github/workflows/check-docs-sidebar.yml | New CI workflow that runs the sidebar check; BASE_REF derivation handles PR/push/dispatch correctly, but path-filtering means the workflow is optional rather than mandatory for all PRs. |
| bin/ci-local | Moves FAILED_JOBS/run_job declarations before the early-exit paths and wires in the sidebar check for docs-only changes and the normal full-CI path; all code paths run the check exactly once. |
| docs/.sidebar-exclusions | New exclusions file listing legacy redirect stubs and pages linked from intro rather than sidebar; well-commented with clear guidance for future contributors. |
| AGENTS.md | Single-line addition documenting the sidebar requirement and exclusion mechanism; clear and actionable for AI agents and human contributors. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[CI trigger: push/PR/dispatch] --> B{Path filter\ndocs/oss/** or docs/pro/**?}
B -- No --> Z[Workflow skipped]
B -- Yes --> C[checkout with fetch-depth 0]
C --> D[script/check-docs-sidebar BASE_REF]
D --> E[git diff --diff-filter=A BASE_REF...HEAD]
E --> F{Any new\ndocs/oss or docs/pro\n.md/.mdx files?}
F -- No --> G[✓ Exit 0]
F -- Yes --> H[For each new file\ncompute DOC_ID]
H --> I{ID in\n.sidebar-exclusions?}
I -- Yes --> J[⊘ Skip]
I -- No --> K{ID appears\nin sidebars.ts?}
K -- Yes --> L[✓ Pass]
K -- No --> M[✗ Add to MISSING list]
J & L & M --> N{All files\nprocessed?}
N -- No --> H
N -- Yes --> O{MISSING empty?}
O -- Yes --> P[✓ Exit 0]
O -- No --> Q[✗ Exit 1 — list missing IDs]
Reviews (1): Last reviewed commit: "feat: add CI guard requiring sidebar upd..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ec91d8625
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/ci-local`:
- Around line 45-50: The run_job function currently uses eval "$command", which
enables command injection when user-controlled values like BASE_REF are
embedded; change run_job to take the job name and command arguments (e.g.,
run_job job_name -- script/check-docs-sidebar arg1 arg2) and execute them with
an argv-based call (use "$@" for the command invocation) instead of eval; update
all call sites that currently build a single quoted string (the
script/check-docs-sidebar invocations at the spots mentioned) to pass the
program and its arguments as separate parameters so no user input is subject to
shell re-parsing.
In `@script/check-docs-sidebar`:
- Line 43: The assignment to ADDED_FILES currently swallows git errors by
falling back to an empty string; update the ADDED_FILES capture to fail the
script when `git diff --name-only --diff-filter=A "$BASE_REF"..."$CURRENT_REF"`
returns a non-zero exit (remove the `|| echo ""` fallback and/or check the
command's exit status), so any git diff failure causes the script to exit
non-zero and not silently proceed; locate the ADDED_FILES assignment and adjust
it and any subsequent logic that assumes a successful diff (references:
ADDED_FILES, BASE_REF, CURRENT_REF).
- Line 63: The script uses Bash associative arrays (declare -A EXCLUSION_MAP)
which require Bash 4+, so add a Bash version check at the top of the affected
scripts (script/check-docs-sidebar and bin/ci-rerun-failures; note bin/ci-local
invokes them) that verifies BASH_VERSINFO[0] is >= 4 and prints a clear error
instructing macOS users to upgrade Bash (or run with a newer shell), then exit
non‑zero; additionally add a short note to the README or CONTRIBUTING docs
stating the Bash 4+ requirement so contributors know to install/enable a newer
Bash before running local CI scripts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7c0ffdf9-89b0-4e8c-a23b-e0cc1a8e2695
📒 Files selected for processing (5)
.github/workflows/check-docs-sidebar.ymlAGENTS.mdbin/ci-localdocs/.sidebar-exclusionsscript/check-docs-sidebar
|
WIP — picking this up. |
Address review feedback with empirically-reproduced fixes: - script/check-docs-sidebar: exit hard on fetch/diff errors instead of silently reporting "no new docs" (matches ci-changes-detector). Add base-ref existence validation. Change --diff-filter=A to AR so pure renames are caught. Include uncommitted (staged + unstaged) files in local dev via `git diff HEAD`, not just untracked via ls-files. - bin/ci-local: make run_job variadic so new callers can pass argv without eval. Legacy single-string callers unchanged. Update the four new sidebar-check call sites to pass BASE_REF as a separate argument (fixes a quoting brittleness when refs contain single quotes). - docs/.sidebar-exclusions: add getting-started/comparison-with-alternatives and getting-started/nextjs-with-separate-rails-backend — intentionally removed from the sidebar in #3065 as deep-dive companions to the Decision Guide, kept published as supporting material. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
bin/ci-local (1)
44-73:⚠️ Potential issue | 🟠 Major
run_jobstill keeps anevalexecution path.Line 56 preserves the same injection footgun that was previously flagged. Even with argv usage for docs checks, any future string-form caller can reintroduce command parsing risk. Prefer argv-only execution and migrate remaining string-form call sites.
#!/bin/bash set -euo pipefail # Verify eval path still exists and identify legacy string-form callers. rg -n 'eval "\$1"' bin/ci-local rg -nP 'run_job\s+"[^"]+"\s+"[^"]+"' bin/ci-local🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ci-local` around lines 44 - 73, The run_job function currently retains a legacy eval-based execution path (the eval "$1" branch) which reintroduces shell injection risk; remove the eval branch and enforce argv-only invocation in run_job (accept job_name then command+args and invoke them as "$@"), update or migrate any legacy string-form callers (calls matching run_job "Name" "command string") to the argv form (run_job "Name" cmd arg1 ...), and optionally add a defensive check inside run_job to error out if only a single non-empty argument is passed so future callers cannot rely on a single-string eval path; target the run_job function and all call sites that use the quoted single-string form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/ci-local`:
- Around line 49-73: The script exits early because run_job failures trigger set
-euo pipefail; for any invocation of run_job that is followed by a later check
of FAILED_JOBS you must allow the command to fail without exiting the script —
change those calls to append "|| true" (e.g., replace calls like run_job "name"
... with run_job "name" ... || true) so FAILED_JOBS aggregation and subsequent
checks run; apply this change to every run_job invocation that precedes the
FAILED_JOBS post-check branches (the calls referenced around the three
post-check blocks) so the script continues to the aggregate failure logic.
---
Duplicate comments:
In `@bin/ci-local`:
- Around line 44-73: The run_job function currently retains a legacy eval-based
execution path (the eval "$1" branch) which reintroduces shell injection risk;
remove the eval branch and enforce argv-only invocation in run_job (accept
job_name then command+args and invoke them as "$@"), update or migrate any
legacy string-form callers (calls matching run_job "Name" "command string") to
the argv form (run_job "Name" cmd arg1 ...), and optionally add a defensive
check inside run_job to error out if only a single non-empty argument is passed
so future callers cannot rely on a single-string eval path; target the run_job
function and all call sites that use the quoted single-string form.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7af4ec37-d7de-4a40-96f3-9a4d7231320e
📒 Files selected for processing (3)
bin/ci-localdocs/.sidebar-exclusionsscript/check-docs-sidebar
✅ Files skipped from review due to trivial changes (1)
- docs/.sidebar-exclusions
🚧 Files skipped from review as they are similar to previous changes (1)
- script/check-docs-sidebar
Review: CI guard for docs sidebar entriesOverall this is a clean, well-structured addition. The script is readable, the exclusion mechanism is a good escape hatch, and wiring it into both Bug (must fix)Local dev mode checks modified docs, not just new ones (
Should fix
The three-dot Implicit eval/argv heuristic in The Minor / informational
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cd974a1a7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- script/check-docs-sidebar: add --diff-filter=AR to the local uncommitted diff. Without it, editing any pre-existing doc absent from sidebars.ts (e.g. a release-notes file) would trigger a false local failure even though CI (which uses --diff-filter=AR on the committed diff) would pass. Verified empirically: editing docs/oss/upgrading/release-notes/16.4.0.md previously failed; now passes. - bin/ci-local: split the variadic run_job helper into two explicit forms. run_job_argv takes argv and invokes directly (no shell re-parsing, safe with variables). _run_job_shell keeps the legacy single-string shell path for compound commands like "cd X && make" that cannot be expressed as argv. run_job is a back-compat wrapper that delegates to _run_job_shell, so the 18 existing legacy callers are untouched. The 4 new sidebar-check call sites use run_job_argv for unambiguous argv semantics. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address-review summaryScan scope: full PR history (no prior summary comment existed). MatteredRound 1 (pre-fix):
Round 2 (post-fix, by claude + chatgpt-codex):
Deferred to follow-up issues:
Not addressed in this PR (out of scope):
Skipped
Follow-up issues
VerificationEmpirically re-tested after 1d83b3e: baseline (no changes), untracked new doc, staged new doc, editing existing doc (regression check), bad base ref, and rename detection — all five tests pass. Both Fix commits
All 17 review threads are resolved. Next default scan starts after this comment. Say |
Review: feat: add CI guard requiring sidebar updates when adding docsOverall this is a solid addition — enforcing sidebar registration via CI is exactly the right approach to prevent orphaned docs, and the script is well-structured with good error handling and a clear exclusion mechanism. A few issues worth addressing before merging: Bugs / correctness
Minor concerns
What's good
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
bin/ci-local (2)
112-117: De-duplicate the docs-only sidebar-check exit path.The same block appears in three branches. A small helper would reduce drift risk and simplify future edits.
Also applies to: 128-132, 153-157
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ci-local` around lines 112 - 117, The docs-only sidebar-check block is duplicated; refactor by adding a small helper function (e.g., run_docs_sidebar_check) that encapsulates running run_job_argv "Docs Sidebar Check" script/check-docs-sidebar "$BASE_REF", printing the "${GREEN}No further CI needed for documentation-only changes!${NC}" message, and checking the FAILED_JOBS array to exit with status 1 if needed; then replace the three duplicated blocks with calls to run_docs_sidebar_check to avoid drift and centralize the logic.
217-219: Run the sidebar check before dependency installation for faster failure.
Line 217says this check is fast/no-deps, but it currently runs after dependency install. Moving it above the install block would fail faster when sidebar coverage is missing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/ci-local` around lines 217 - 219, The Docs Sidebar Check currently runs after dependencies are installed; move the run_job_argv invocation for the sidebar check (the line starting with run_job_argv "Docs Sidebar Check" script/check-docs-sidebar "$BASE_REF") so it executes before the dependency-install step (i.e. before the script/step that installs dependencies or the job that performs dependency installation) so the fast/no-deps check fails early.script/check-docs-sidebar (1)
79-83: Trim leading whitespace in exclusion entries to prevent silent misses.
Line 82trims trailing whitespace only. A leading space indocs/.sidebar-exclusionswould create a non-matching key.♻️ Proposed hardening
while IFS= read -r line; do # Strip inline comments and trailing whitespace line="${line%%#*}" + line="${line#"${line%%[![:space:]]*}"}" line="${line%"${line##*[![:space:]]}"}" # Skip blank lines [ -z "$line" ] && continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@script/check-docs-sidebar` around lines 79 - 83, The loop that reads exclusions (the while IFS= read -r line; do ... done) only strips inline comments and trailing whitespace from the variable line, so entries in docs/.sidebar-exclusions with leading spaces will not match; update the read loop to also remove leading whitespace from line (using shell parameter expansion to trim leading spaces) after stripping comments and trailing whitespace so entries are normalized before any matching logic runs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/ci-local`:
- Around line 112-117: The docs-only sidebar-check block is duplicated; refactor
by adding a small helper function (e.g., run_docs_sidebar_check) that
encapsulates running run_job_argv "Docs Sidebar Check" script/check-docs-sidebar
"$BASE_REF", printing the "${GREEN}No further CI needed for documentation-only
changes!${NC}" message, and checking the FAILED_JOBS array to exit with status 1
if needed; then replace the three duplicated blocks with calls to
run_docs_sidebar_check to avoid drift and centralize the logic.
- Around line 217-219: The Docs Sidebar Check currently runs after dependencies
are installed; move the run_job_argv invocation for the sidebar check (the line
starting with run_job_argv "Docs Sidebar Check" script/check-docs-sidebar
"$BASE_REF") so it executes before the dependency-install step (i.e. before the
script/step that installs dependencies or the job that performs dependency
installation) so the fast/no-deps check fails early.
In `@script/check-docs-sidebar`:
- Around line 79-83: The loop that reads exclusions (the while IFS= read -r
line; do ... done) only strips inline comments and trailing whitespace from the
variable line, so entries in docs/.sidebar-exclusions with leading spaces will
not match; update the read loop to also remove leading whitespace from line
(using shell parameter expansion to trim leading spaces) after stripping
comments and trailing whitespace so entries are normalized before any matching
logic runs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eb406432-3a6f-440e-b65c-624ed07493b9
📒 Files selected for processing (2)
bin/ci-localscript/check-docs-sidebar
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d83b3e7bb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- Strip leading whitespace in the exclusion parser. An entry written as ` doc-id` (with accidental leading space) was previously stored verbatim in EXCLUSION_MAP and failed to match a lookup for `doc-id`, producing a silent non-exclusion → false CI failure. Verified empirically. Trivial one-line bash parameter-expansion fix. - Make the `git fetch origin` call best-effort and move the hard-fail gate to the subsequent `git rev-parse --verify` check. The real invariant is "base ref must be resolvable locally", not "the fetch call must succeed". The previous hard-fail broke offline local development even when a cached origin/main ref existed and would have produced a correct diff. Verified against four scenarios: online+valid, offline+cached, online+bad, offline+bad — all behave correctly. This is strictly better than the previous behavior: preserves the anti-silent-pass protection from round 1 while removing the offline-dev lockout introduced by round 2. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address-review summaryScan scope: activity after 2026-04-11T00:57:25Z (previous checkpoint). Mattered
Skipped
Fix commit
All 24 review threads are resolved. Next default scan starts after this comment. Say |
Review: CI guard for docs sidebar entriesThe overall approach is sound — detect added/renamed docs, derive sidebar IDs, check against Bugs1. Pre-existing untracked docs trigger false positives locally (
2. Commented-out entries in
3.
Minor
Otherwise this is clean work: good use of |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acc0fc4ec8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
- script/check-docs-sidebar: strip single-line TypeScript comments from SIDEBAR_CONTENT before the quoted-string grep. A comment like `// TODO: re-add 'getting-started/foo' later` could previously satisfy the presence check, causing a silent pass even though the doc ID was not in any real sidebar category. The current sidebars.ts uses bare bullet comments (`// - misc/doctrine`), so this was a latent concern rather than a reachable bug — but it has been reported across four review rounds and the minimal fix is one grep line. Block comments are not handled; the convention in this file is single-line comments only. Verified empirically: injected a fake ghost ID inside a TODO comment, confirmed the grep no longer false-matches it. - bin/ci-local: add `|| true` to the always-run sidebar check call. This call is architecturally unique among the 22 run_job_* invocations — it is the only one at the top level of the main path that is not gated by an `if "$RUN_X" = true` block. Without `|| true`, a sidebar failure would trigger `set -euo pipefail` and abort all subsequent lint/test jobs, forcing contributors to fix sidebar first then discover lint/test failures in a second round trip. The FAILED_JOBS array is still populated inside run_job_argv, so the end-of-script summary correctly reports the sidebar failure along with any other failures. The 21 gated legacy callers are untouched; any broader fail-fast vs aggregate redesign belongs in a separate PR. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address-review summaryScan scope: activity after 2026-04-11T01:17:03Z (previous checkpoint). Mattered
Declined (with rationale)
Skipped (not a bug in current state)
Fix commit
All 28 review threads are resolved. Next default scan starts after this comment. Say |
Review: feat: add CI guard requiring sidebar updates when adding docsOverall this is a well-designed guard. The script is clearly structured, handles edge cases (shallow clones, offline dev, zero-SHA initial push, renamed files, inline comments in One substantive issueDead code in the docs-only early-exit paths ( The The main-path call (line 224) correctly uses Minor observations
Nits
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c48f494239
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
…it paths
The three docs-only early-exit branches each had:
run_job_argv "Docs Sidebar Check" script/check-docs-sidebar "$BASE_REF"
echo -e "${GREEN}No further CI needed..."
if [ ${#FAILED_JOBS[@]} -gt 0 ]; then
exit 1
fi
exit 0
Under `set -euo pipefail`, this was confused control flow: a failing
run_job_argv immediately exited the script via set -e (before reaching
the FAILED_JOBS check), and on success the FAILED_JOBS array was
trivially empty. The `if FAILED_JOBS > 0` block was dead code that
made it look like FAILED_JOBS drove the exit code when it didn't.
Replace each block with a simple pattern: let set -e do its job on
failure, explicit exit 0 on success, with a comment making the
reliance on set -e obvious. No behavior change — both paths already
did the right thing — but the code is honest about it now.
Note: this is specifically *not* the same fix as the always-run call
at the main path (line 224), which uses `|| true` so that subsequent
lint/test jobs can still run. The early-exit paths have no subsequent
jobs, so set -e is the simpler and more correct choice there.
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Address-review summaryScan scope: activity after 2026-04-11T01:37:51Z (previous checkpoint). Mattered
Skipped
Fix commit
All 32 review threads are resolved. Next default scan starts after this comment. Say |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 084720206f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| # a `// comment` (e.g. `// TODO: re-add 'foo'`) cannot satisfy the grep below. | ||
| # Block comments `/* ... */` are not handled; the convention in this file is | ||
| # single-line comments only. | ||
| SIDEBAR_CONTENT=$(grep -v '^[[:space:]]*//' "$SIDEBAR_FILE") |
There was a problem hiding this comment.
SIDEBAR_CONTENT only removes lines that start with //, so quoted IDs that appear in inline comments (for example items: [...], // TODO add 'new/doc') are still searchable by the later grep -qF checks. In that case a newly added doc can pass this guard without an actual sidebar entry, which weakens the CI protection this script is intended to provide.
Useful? React with 👍 / 👎.
|
This is solid work — the design is well-thought-out and hardened across several review rounds. Key choices are good: Three observations (also posted inline). None are blockers — the logic is correct for the current 1. Inline The script strips full-line 'other-doc', // TODO: re-add 'getting-started/foo' when X ships…would produce a false pass for 2. Local dev path flags untracked WIP docs ( The local path includes ALL untracked 3. Push-to-main event: For |
| # a `// comment` (e.g. `// TODO: re-add 'foo'`) cannot satisfy the grep below. | ||
| # Block comments `/* ... */` are not handled; the convention in this file is | ||
| # single-line comments only. | ||
| SIDEBAR_CONTENT=$(grep -v '^[[:space:]]*//' "$SIDEBAR_FILE") |
There was a problem hiding this comment.
This strips full-line // comments (where // is the first non-whitespace token), which correctly blocks a ghost match from:
// TODO: re-add 'getting-started/foo' laterHowever, it does not strip inline // comments on data lines. A line like:
'other-doc', // TODO: restore 'getting-started/foo' when X ships…would cause 'getting-started/foo' to appear in SIDEBAR_CONTENT and satisfy the grep -qF check on line 127, producing a false pass even though getting-started/foo is not in any actual sidebar category.
The code comment acknowledges block comments are out of scope, but this inline-comment case is reachable with typical TypeScript style. A simple fix:
| SIDEBAR_CONTENT=$(grep -v '^[[:space:]]*//' "$SIDEBAR_FILE") | |
| SIDEBAR_CONTENT=$(grep -v '^[[:space:]]*//' "$SIDEBAR_FILE" | sed 's|[[:space:]]//.*||') |
Caveat: if a sidebar ID ever contained // (it shouldn't — Docusaurus IDs are path segments), this would cause a false negative. The tradeoff is acceptable given how sidebar IDs are formed.
| if [ -z "${CI:-}" ] && [ "$CURRENT_REF" = "HEAD" ]; then | ||
| UNCOMMITTED=$(git diff --name-only --diff-filter=AR HEAD 2>/dev/null || echo "") | ||
| UNTRACKED=$(git ls-files --others --exclude-standard 2>/dev/null || echo "") | ||
| ADDED_FILES=$(printf "%s\n%s\n%s" "$ADDED_FILES" "$UNCOMMITTED" "$UNTRACKED" | sort -u | grep -v '^$' || echo "") |
There was a problem hiding this comment.
The git ls-files --others --exclude-standard on line 65 includes all untracked files — not just ones that are staged or intended for the current commit. This means a developer who has a work-in-progress draft at docs/oss/new-feature.md (not yet staged) will immediately see a local sidebar-check failure, even if they were not trying to land that file right now.
Two options:
Option A — staged only (mirrors what CI sees, no surprise failures from drafts):
| if [ -z "${CI:-}" ] && [ "$CURRENT_REF" = "HEAD" ]; then | |
| UNCOMMITTED=$(git diff --name-only --diff-filter=AR HEAD 2>/dev/null || echo "") | |
| UNTRACKED=$(git ls-files --others --exclude-standard 2>/dev/null || echo "") | |
| ADDED_FILES=$(printf "%s\n%s\n%s" "$ADDED_FILES" "$UNCOMMITTED" "$UNTRACKED" | sort -u | grep -v '^$' || echo "") | |
| if [ -z "${CI:-}" ] && [ "$CURRENT_REF" = "HEAD" ]; then | |
| UNCOMMITTED=$(git diff --name-only --diff-filter=AR HEAD 2>/dev/null || echo "") | |
| STAGED_NEW=$(git diff --cached --name-only --diff-filter=AR 2>/dev/null || echo "") | |
| ADDED_FILES=$(printf "%s\n%s\n%s" "$ADDED_FILES" "$UNCOMMITTED" "$STAGED_NEW" | sort -u | grep -v '^$' || echo "") | |
| fi |
Option B — keep untracked, add a note to the error message so contributors understand why an unstaged draft triggered the failure.
Option A is safer for developer UX; Option B is more eager (catches the omission as early as possible).
|
|
||
| - name: Check new docs have sidebar entries | ||
| env: | ||
| BASE_REF: ${{ github.event.pull_request.base.sha || github.event.before || 'origin/main' }} |
There was a problem hiding this comment.
For push events, github.event.before is a raw 40-character commit SHA (e.g. a1b2c3d...). The check-docs-sidebar script only triggers its git fetch when BASE_REF starts with origin/ (line 39 of the script), so for push events the fetch is entirely skipped. The script then relies on fetch-depth: 0 (line 30 above) having already made the SHA available locally.
This works correctly, but the coupling is silent — someone reading the script in isolation wouldn't know why a bare SHA is always resolvable in CI. A one-line comment in the workflow step would make it explicit:
| BASE_REF: ${{ github.event.pull_request.base.sha || github.event.before || 'origin/main' }} | |
| env: | |
| BASE_REF: ${{ github.event.pull_request.base.sha || github.event.before || 'origin/main' }} | |
| # fetch-depth: 0 above ensures bare SHAs (push events) are always resolvable; | |
| # the script's git-fetch only fires for 'origin/*' refs (pull_request / workflow_dispatch). | |
| run: script/check-docs-sidebar "$BASE_REF" |
2 participants
Summary
script/check-docs-sidebarthat detects newly added docs underdocs/oss/anddocs/pro/and verifies each has a corresponding entry indocs/sidebars.tsdocs/.sidebar-exclusionswith existing intentional exclusions (legacy redirects, pages linked from intro instead of sidebar).github/workflows/check-docs-sidebar.yml) and local CI (bin/ci-local)AGENTS.mdBoundaries/Always section with the new sidebar ruleCloses #2889
Test plan
docs/oss/X.md→ sidebar IDXanddocs/pro/X.md→ sidebar IDpro/X.sidebar-exclusionsare skipped🤖 Generated with Claude Code
Note
Medium Risk
Adds a new CI gate and local CI step that can fail PRs/commits based on git diff detection, so mis-detection of added docs or base-ref handling could block merges.
Overview
Introduces a new guard (
script/check-docs-sidebar) that detects newly added published docs and fails if their derived doc IDs are not present indocs/sidebars.ts, with an opt-out list indocs/.sidebar-exclusions.Wires the check into GitHub Actions via a new
check-docs-sidebar.ymlworkflow and intobin/ci-local(runs always and is the only check run for docs-only change sets), and updatesAGENTS.mdto document the new requirement.Reviewed by Cursor Bugbot for commit 8ec91d8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Documentation
Chores