Refactor governance around version milestones - #766
Conversation
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (24)
📝 WalkthroughWalkthroughThe PR replaces goalpost and version-label scheduling with plain ChangesRelease governance migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
🔍 The Case of Pull Request #766Schema Sets
Schema Set
|
![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: db87ccff1f
ℹ️ 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/smoke/repo-bats-prepush.sh (1)
28-34: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPre-push run inherits the same 3-minute cargo build risk.
test/release-governance.batsinvokescargo test --locked -p xtask; on a cold target directory this can exceedtimeout 3mand fail the push for reasons unrelated to governance.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/smoke/repo-bats-prepush.sh` around lines 28 - 34, Increase or otherwise adjust the timeout applied in the loop running the Bats files so test/release-governance.bats can complete its cold cargo test --locked -p xtask build without premature termination, while preserving the existing timeout enforcement and invocation behavior for the other files.
🧹 Nitpick comments (2)
xtask/src/main.rs (1)
2042-2051: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
GITHUB_REPOSITORYvs origin comparison is case-sensitive.GitHub owner/repo names are case-insensitive, so a remote URL whose casing differs from the canonical
GITHUB_REPOSITORYvalue (or vice versa) fails the guard and blocks a release for no real reason. Consider comparing case-insensitively while still returning the origin-derived value.♻️ Suggested comparison tweak
- if repository != origin_repository { + if !repository.eq_ignore_ascii_case(&origin_repository) { return Err(format!( "GITHUB_REPOSITORY `{repository}` does not match origin fetch and push repository `{origin_repository}`" )); }The same applies to the fetch-vs-push comparison on Line 2035.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@xtask/src/main.rs` around lines 2042 - 2051, Update the repository comparisons in the ambient GITHUB_REPOSITORY guard and the fetch-vs-push validation to compare owner/repository paths case-insensitively. Preserve the existing origin-derived repository value and error messages, changing only the equality checks around parse_github_repository_path and the fetch/push comparison.test/release-governance.bats (1)
126-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShell suite now drives a Rust test build.
Wrapping
cargo test -p xtaskin a Bats case couples the governance suite to Rust compilation (and drives the timeout risk noted in.github/workflows/ci.yml). Sincecargo xtask preflight/cargo testalready runs the same assertion, consider dropping this case and letting the Rust suite own it, or asserting the YAML invariants directly withgrep/rgas the neighbouring tests do.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/release-governance.bats` around lines 126 - 131, The Bats case “release governance YAML is structurally valid” unnecessarily invokes the Rust build via cargo test. Remove this test and rely on the existing Rust suite, or replace its cargo invocation with direct YAML invariant checks using the grep/rg pattern used by neighboring tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.continuum/release.yml:
- Line 123: Update classification_axis in .continuum/release.yml at lines
123-123 to explicitly distinguish label axes from the project fields priority
and status. In docs/topics/contributing/triage.md at lines 79-86, remove
priority:* from the topic-label list; retain status as a project field and leave
other label categories unchanged.
- Around line 120-122: Apply one exhaustive scheduling predicate across all
listed governance surfaces: scheduled state must prohibit every retired lane:*
label, triage:* label, concrete version label, and extra milestone while
retaining exactly one v{version} milestone; unscheduled state must require
exactly one triage:* label, no milestone, and no retired or concrete version
labels. Update .continuum/release.yml lines 120-122,
.github/pull_request_template.md line 31, both AGENTS.md ranges 37-40 and 63-65,
and docs/topics/contributing/first-pr.md lines 46 and 91-95 with the
corresponding prohibition.
In `@docs/architecture/hosts.md`:
- Around line 41-44: Update the documentation text describing live planning
state so Issues, Milestones, and Projects are the planning surfaces, with plain
vX.Y.Z milestones as the release-scheduling authority. Describe classification
labels separately as work-classification metadata, not as scheduling or
planning-state surfaces.
In `@docs/CRATES_IO_RELEASE.md`:
- Around line 274-282: Fix the ordered-list numbering in the affected continued
lists, including the sections around the release steps and lines 302–318, so
they satisfy MD029 across fenced code blocks. Either restart numbering where
markdownlint detects a new list or indent the intervening code fences to keep
each block within its preceding list item; preserve the documented step order.
In `@docs/METHOD.md`:
- Around line 39-47: Scope all scheduling and release invariants to current open
work, explicitly preserving historical closed milestones, issues, and labels.
Update the Scheduled Slice and Release rules in docs/METHOD.md, queue states in
CONTRIBUTING.md, hierarchy language in docs/BEARING.md, release contract in
docs/governance/RELEASE_POLICY.md, guard preconditions in
docs/method/release-runbook.md, migration wording in
docs/governance/RELEASE_CHECKLIST.md, doctrine in docs/method/release.md, and
release-shape rule in docs/topics/releases.md; each site requires this
qualification.
In `@docs/topics/contributing/triage.md`:
- Around line 88-131: Rewrite the “One-Time Live Cutover” procedure so
governance PR merge occurs before any live GitHub mutation: perform a
post-merge, read-only audit and record the complete mapping first, then execute
the live migration, followed by verification. Keep the existing migration rules
and failure handling, but ensure failed audits or migrations do not leave
enforcement unmerged with partially changed tracker state.
In `@test/README.md`:
- Line 60: Update the CI gating sentence immediately above the test suite list
in test/README.md to state that the suites run unconditionally, matching the
current workflow and test/ci-workflows.bats assertion; leave the suite list
unchanged.
In `@test/release-governance.bats`:
- Around line 273-286: Require every marker variable to be non-zero before
evaluating the ordering chain in both awk assertions:
test/release-governance.bats lines 273-286 must validate preflight, gate, prep,
refresh, unchanged, tag, tagged, push, and workflow; lines 332-345 must validate
sync, record, preflight, gate, clear, refresh, unchanged, synced, and tag.
Preserve the existing ordering checks after these presence validations.
- Around line 126-131: The Bats test’s nested cargo invocation can exceed
per-file timeouts on a cold Rust build. In test/release-governance.bats lines
126-131, remove the release_governance_yaml_is_structurally_valid case or
replace it with direct grep/rg YAML invariant checks; no direct change is
required in .github/workflows/ci.yml lines 60-65 or
scripts/smoke/repo-bats-prepush.sh lines 28-34 if the case is removed.
---
Outside diff comments:
In `@scripts/smoke/repo-bats-prepush.sh`:
- Around line 28-34: Increase or otherwise adjust the timeout applied in the
loop running the Bats files so test/release-governance.bats can complete its
cold cargo test --locked -p xtask build without premature termination, while
preserving the existing timeout enforcement and invocation behavior for the
other files.
---
Nitpick comments:
In `@test/release-governance.bats`:
- Around line 126-131: The Bats case “release governance YAML is structurally
valid” unnecessarily invokes the Rust build via cargo test. Remove this test and
rely on the existing Rust suite, or replace its cargo invocation with direct
YAML invariant checks using the grep/rg pattern used by neighboring tests.
In `@xtask/src/main.rs`:
- Around line 2042-2051: Update the repository comparisons in the ambient
GITHUB_REPOSITORY guard and the fetch-vs-push validation to compare
owner/repository paths case-insensitively. Preserve the existing origin-derived
repository value and error messages, changing only the equality checks around
parse_github_repository_path and the fetch/push comparison.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 6aa021f4-45df-4e94-82f9-773c4d697221
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (42)
.continuum/release.yml.github/ISSUE_TEMPLATE/bug.yml.github/ISSUE_TEMPLATE/chore.yml.github/ISSUE_TEMPLATE/feature.yml.github/ISSUE_TEMPLATE/rfc.yml.github/pull_request_template.md.github/workflows/ci.yml.github/workflows/release-crates.ymlAGENTS.mdCHANGELOG.mdCONTRIBUTING.mdRELEASE.mddocs/BEARING.mddocs/CRATES_IO_RELEASE.mddocs/JEDIT_CAPABILITY_EVIDENCE.mddocs/METHOD.mddocs/README.mddocs/architecture/hosts.mddocs/design/0019-weslaw-semantic-law-ir/weslaw-semantic-law-ir.mddocs/design/0020-holmes-weslaw-assurance-prd-test-plan/holmes-weslaw-assurance-prd-test-plan.mddocs/design/README.mddocs/design/TEMPLATE.mddocs/governance/DOCUMENTATION_STANDARD.mddocs/governance/RELEASE_CHECKLIST.mddocs/governance/RELEASE_POLICY.mddocs/governance/labels.mddocs/method/guide.mddocs/method/process.mddocs/method/release-runbook.mddocs/method/release.mddocs/site/roadmap.mddocs/topics/README.mddocs/topics/contributing/first-pr.mddocs/topics/contributing/triage.mddocs/topics/releases.mdscripts/smoke/repo-bats-prepush.shscripts/test-ci-locally.shtest/README.mdtest/ci-workflows.batstest/release-governance.batsxtask/Cargo.tomlxtask/src/main.rs
💤 Files with no reviewable changes (1)
- .github/workflows/release-crates.yml
Code Lawyer self-audit findings
These are additive to the 10 unresolved inline threads and the unique global case-insensitive repository-comparison finding. I will process the merged queue one issue at a time with deterministic regression evidence and one commit per issue. Cc: @codex |
Summary
Testing
|
Code Lawyer self-audit addendum
Cc: @codex for second opinion. |
Code Lawyer Activity Summary
Verification
Phase IV merge judgment remains pending live CI, approvals, CodeRabbit cooldown, and the full local format gate. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5404a6b10
ℹ️ 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".
| - name: install bats, jq, and ripgrep | ||
| shell: bash | ||
| run: sudo apt-get install -y bats jq | ||
| run: sudo apt-get install -y bats jq ripgrep |
There was a problem hiding this comment.
When a job follows docs/ci.md:22-29, GitHub rejects the advertised snippet because reusable workflows must be invoked at jobs.<job_id>.uses, not as a step; invoking it correctly as a job still runs .github/workflows/install-bats.yml:9-20 on a separate runner, so its packages are unavailable to the caller job. Checked .github/workflows/ci.yml:31-33, which already uses the checked-in composite action, and this new ripgrep dependency also makes the duplicate workflow diverge because it installs only Bats and jq. Remove the reusable workflow and document ./.github/actions/install-bats as the sole installer.
Useful? React with 👍 / 👎.
1 participant
Linked Issue
Closes #742
Summary
Replace Wesley's split goalpost/version-label scheduling model with plain semantic-version GitHub milestones as the sole scheduling authority.
vX.Y.Zmilestone.triage:*label and no milestone.legend:*,pkg:*,work:*, and ordinary type labels retain classification.Why
PR #741 and issue #740 exposed an ambiguity in the existing model: a concrete release-relevant slice could plausibly fit several slogan goalposts, while the separate
vX.Y.Zlabel already carried the actual scheduling decision. The duplicated axes added subjective classification and could block otherwise green work without adding release information.This change makes the tracker model deterministic and gives release guards one authoritative query surface.
Changes
AGENTS.md,docs/BEARING.md, anddocs/METHOD.md..continuum/release.ymlto declare version milestones as the only scheduling axis.Live Migration Boundary
This PR changes repository doctrine, validation, templates, and release behavior only.
It does not mutate the live GitHub tracker. After this PR is approved and merged, the operator migration is:
Release: vX.Y.Zmilestones to plainvX.Y.Z.triage:*issues without milestones.v0.3.0.The migration must be read-only dry-run audited before any live mutation and verified afterward.
Validation
git diff --check origin/main...HEAD— passed.cargo test --locked -p xtask— 67 passed.bats -t test/release-governance.bats test/ci-workflows.bats— 63 passed.pnpm run preflight— passed.Risk
The principal risk is divergence between merged repository doctrine and live tracker metadata during the transition. The explicit post-merge migration sequence, fail-closed release queries, and deterministic dry run bound that risk.
No live issue, milestone, label, project, or release state is changed by this PR.
Backout
Revert the merge commit before performing the live tracker migration. If the migration has already begun, stop and restore the recorded pre-migration issue/milestone assignments through ordinary forward mutations; do not rewrite Git history.
Merge Strategy
Merge commit only. No rebase and no force push.
Summary by CodeRabbit
Release Management
vX.Y.Zmilestone.Documentation
Validation