Skip to content

fix(ci): enforce legacy docs/ gate in Lint workflow#26322

Merged
Kangyan-Zhou merged 3 commits into
sgl-project:mainfrom
zijiexia:fix/docs-gate-ci-enforcement
May 26, 2026
Merged

fix(ci): enforce legacy docs/ gate in Lint workflow#26322
Kangyan-Zhou merged 3 commits into
sgl-project:mainfrom
zijiexia:fix/docs-gate-ci-enforcement

Conversation

@zijiexia
Copy link
Copy Markdown
Collaborator

@zijiexia zijiexia commented May 25, 2026

Motivation

The pre-commit hook check-no-docs-changes (added in 8750885) was meant to reject changes under the legacy docs/ tree, but it has been silently bypassed on every PR since it landed. Recent examples include #24593, #23329, #25120, #19290, #24491, #24874, #24716, #23708, #24330, #20922, #24005, #24329, #24188, and #23199 — all merged with docs/ modifications despite the hook being "enabled".

Root cause: the hook reads git diff --cached --name-only, which is empty on a fresh CI checkout. pre-commit run --all-files in the Lint workflow therefore runs the script with no staged paths and the script exits 0. The hook only enforces the rule locally for contributors with pre-commit installed; everyone else (or anyone using --no-verify) bypasses it.

Modifications

.github/workflows/lint.yml:

  • actions/checkout@v4: set fetch-depth: 0 so the merge base is available for the three-dot diff.
  • New step Reject changes under legacy docs/: on pull_request events, diffs origin/<base_ref>...HEAD and feeds the changed paths to the existing scripts/ci/check_no_docs_changes.py. Placed before Python setup so we fail fast without paying for irrelevant tooling.
  • Guards against silent failures: an explicit if ! around the diff capture aborts on git diff errors (e.g., missing merge base) instead of treating empty stdout as "no changes". git fetch no longer uses --depth=1, which could shallow origin/<base_ref> and break the merge base — discovered while validating this PR with act.

Not modified: .pre-commit-config.yaml (the local hook continues to enforce on developer machines), scripts/ci/check_no_docs_changes.py (already accepts paths via sys.argv, so the allowlist — docs/conf.py, docs/_static/css/custom_log.css, docs/_static/js/deprecation_banner.js — is honored automatically).

Accuracy Tests

N/A (CI workflow change).

Speed Tests and Profiling

N/A.

Verification

  • actionlint .github/workflows/lint.yml reports no new findings (one pre-existing setup-python@v4 warning unrelated to this PR).
  • Local script harness:
    • docs/foo.md → exits 1 with rejection message.
    • docs/conf.py → exits 0 (allowlist).
    • python/sglang/srt/__init__.py → exits 0.
  • End-to-end with act + Docker (catthehacker/ubuntu:act-latest runner): a probe commit modifying docs/references/environment_variables.md causes the new step to exit non-zero with:
    Changes under the legacy docs/ directory are not allowed.
    Detected legacy docs/ changes:
      - docs/references/environment_variables.md
    
    This run also surfaced the shallow-fetch / silent-diff-failure bug that the third commit hardens.

Checklist


CI States

Latest PR Test (Base): ❌ Run #26421412193
Latest PR Test (Extra): ❌ Run #26421412119

@gemini-code-assist
Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@Kangyan-Zhou Kangyan-Zhou merged commit a269131 into sgl-project:main May 26, 2026
70 of 74 checks passed
Shunkangz pushed a commit to Shunkangz/sglang that referenced this pull request May 27, 2026
mqhc2020 pushed a commit to mqhc2020/sglang that referenced this pull request Jun 2, 2026
alphabetc1 pushed a commit to alphabetc1/sglang that referenced this pull request Jun 4, 2026
jeynmann pushed a commit to jeynmann/sglang that referenced this pull request Jun 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants