feat(labeling): harden canonical seeding with policy-gated cleanup#451
Conversation
|
Warning Review limit reached
More reviews will be available in 31 minutes and 8 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ 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 |
🔍 Reviewer Summary for PR #451CI Status: ✅ Recommendations
|
github-actions
Bot
added
area:ci
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a label governance policy to manage canonical seeding and gate destructive orphan-label cleanup. It adds a new policy configuration file, updates documentation, and hardens the label synchronization script to defer deletions unless explicitly enabled and approved. It also updates the validation script to enforce canonical label prefixes and validate the new policy schema. The review feedback highlights several key improvements: ensuring cross-platform compatibility on Windows when checking the main module, failing fast in GitHub Actions if required environment variables are missing, fixing a validation bypass for string-based labels, and strictly validating that policy arrays contain only strings.
| if (import.meta.url === `file://${process.argv[1]}`) { | ||
| runCli().catch((error) => { | ||
| console.error(`[label-sync] ${error.message}`); | ||
| process.exit(1); | ||
| }); | ||
| } |
There was a problem hiding this comment.
The comparison of import.meta.url with a file URL constructed from process.argv[1] will fail on Windows because process.argv[1] uses backslashes and does not match the standard file URL format. Use fileURLToPath from the url module to perform a robust, cross-platform main module check.
| if (import.meta.url === `file://${process.argv[1]}`) { | |
| runCli().catch((error) => { | |
| console.error(`[label-sync] ${error.message}`); | |
| process.exit(1); | |
| }); | |
| } | |
| if (process.argv[1] && fileURLToPath(import.meta.url) === path.resolve(process.argv[1])) { | |
| runCli().catch((error) => { | |
| console.error("[label-sync] " + error.message); | |
| process.exit(1); | |
| }); | |
| } |
| import fs from "fs"; | ||
| import path from "path"; | ||
| import yaml from "js-yaml"; | ||
| import github from "@actions/github"; |
There was a problem hiding this comment.
Import fileURLToPath from the built-in url module to enable a robust, cross-platform check for the main module execution.
| import fs from "fs"; | |
| import path from "path"; | |
| import yaml from "js-yaml"; | |
| import github from "@actions/github"; | |
| import fs from "fs"; | |
| import path from "path"; | |
| import yaml from "js-yaml"; | |
| import github from "@actions/github"; | |
| import { fileURLToPath } from "url"; |
| if (!token || !repoSlug) { | ||
| console.log( | ||
| "[label-sync] Skipping sync: requires GITHUB_TOKEN and GITHUB_REPOSITORY.", | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
If the script is executed in a GitHub Actions environment (where process.env.GITHUB_ACTIONS is true), missing required environment variables like GITHUB_TOKEN or GITHUB_REPOSITORY should fail the build rather than silently skipping. This prevents misconfigured workflows from passing silently.
| if (!token || !repoSlug) { | |
| console.log( | |
| "[label-sync] Skipping sync: requires GITHUB_TOKEN and GITHUB_REPOSITORY.", | |
| ); | |
| return; | |
| } | |
| if (!token || !repoSlug) { | |
| if (process.env.GITHUB_ACTIONS === "true") { | |
| throw new Error("Missing GITHUB_TOKEN or GITHUB_REPOSITORY in GitHub Actions environment."); | |
| } | |
| console.log( | |
| "[label-sync] Skipping sync: requires GITHUB_TOKEN and GITHUB_REPOSITORY.", | |
| ); | |
| return; | |
| } |
| const hasAllowedPrefix = allowedPrefixes.some((prefix) => | ||
| item.name.startsWith(prefix), | ||
| ); | ||
| if (!hasAllowedPrefix) { | ||
| fail( | ||
| `Label '${item.name}' must use a canonical family prefix (${allowedPrefixes.join(", ")})`, | ||
| ); | ||
| } |
There was a problem hiding this comment.
The prefix validation is bypassed for string-based labels because of the early return on line 42 (which is outside the modified lines). If labels.yml contains simple string entries, they will not be validated against the allowedPrefixes list. Consider refactoring the validation loop to extract the label name first (supporting both string and object formats) and then applying the prefix check to all entries.
| if ( | ||
| cleanup.approved_orphan_labels !== undefined && | ||
| !Array.isArray(cleanup.approved_orphan_labels) | ||
| ) { | ||
| fail("destructive_cleanup.approved_orphan_labels must be an array"); | ||
| } | ||
|
|
||
| if ( | ||
| cleanup.never_delete_labels !== undefined && | ||
| !Array.isArray(cleanup.never_delete_labels) | ||
| ) { | ||
| fail("destructive_cleanup.never_delete_labels must be an array"); | ||
| } |
There was a problem hiding this comment.
The validation for approved_orphan_labels and never_delete_labels only checks if they are arrays, but does not verify that the elements inside the arrays are strings. If non-string values are accidentally added, they will be silently filtered out by asStringSet in the sync script. It is safer to fail the validation if any element is not a string.
if (
cleanup.approved_orphan_labels !== undefined &&
(!Array.isArray(cleanup.approved_orphan_labels) || !cleanup.approved_orphan_labels.every(item => typeof item === "string"))
) {
fail("destructive_cleanup.approved_orphan_labels must be an array of strings");
}
if (
cleanup.never_delete_labels !== undefined &&
(!Array.isArray(cleanup.never_delete_labels) || !cleanup.never_delete_labels.every(item => typeof item === "string"))
) {
fail("destructive_cleanup.never_delete_labels must be an array of strings");
}
![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: 5dee4bf970
ℹ️ 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".
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
| LABEL_SYNC_REPORT_PATH: .github/reports/labeling/label-sync-${{ github.run_id }}.md |
There was a problem hiding this comment.
When this workflow is manually dispatched with the default dry_run=true, this step now gives the new label-sync CLI a write token but never maps inputs.dry_run to LABEL_SYNC_DRY_RUN (the script only treats LABEL_SYNC_DRY_RUN === "true" as dry-run). As a result a dry-run dispatch can still create or update repository labels before the later labeling agent runs in dry-run mode; please wire the input through here or make the CLI honour the existing DRY_RUN variable.
Useful? React with 👍 / 👎.
| console.log( | ||
| `[label-sync] Created ${syncReport.created.length}, updated ${syncReport.updated.length}, deleted ${syncReport.deleted.length}, deferred ${syncReport.deferredDeletes.length}`, | ||
| ); | ||
| console.log( | ||
| `[label-sync] Deletion mode: ${deletionMode} (policy + env gate)`, | ||
| ); |
There was a problem hiding this comment.
When any create/update API request fails, syncLabelsWithCanonical catches the exception and stores it in syncReport.errors, but this CLI path still writes the report, logs counts, and exits 0. In contexts such as a read-only pull_request token or a rejected label update, .github/workflows/labeling.yml has continue-on-error: false but the job can still go green without actually seeding the canonical labels; please throw when sync errors are present before exiting.
Useful? React with 👍 / 👎.
ashleyshaw
force-pushed
the
codex/449-label-governance-hardening
branch
from
4ae5e74 to
5588826
Compare
🔍 Reviewer Summary for PR #451CI Status: ✅ Recommendations
|
1 participant
Summary
Changes
.github/label-governance-policy.ymlwith destructive cleanup gate (enabled: falsedefault)scripts/agents/includes/label-sync.js:scripts/validation/validate-labeling-configs.cjs:.github/workflows/labeling.yml:GITHUB_TOKENto label syncdocs/MIGRATION.mdwith seeding/cleanup gate rulesValidation
node scripts/validation/validate-labeling-configs.cjsnode scripts/agents/includes/check-template-labels.jsmarkdownlint-cli2on updated markdown filesGate
.github/label-governance-policy.ymlCloses #66
Refs #95
Refs #449