fix(release): retire target labels after tagging - #7118
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRelease labeling now compares merges with the latest release tag and current untagged interval. Serialized release workflows move open stragglers to the next patch label, delete the released label, and update release plans, maintainer guidance, and validation. ChangesRelease label lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Maintainer
participant release-latest-tag
participant retire-release-label-mts
participant GitHub
Maintainer->>release-latest-tag: Confirm release tag and workflow completion
release-latest-tag->>retire-release-label-mts: Pass released tag and repository
retire-release-label-mts->>GitHub: Move open PRs and issues to next patch
retire-release-label-mts->>GitHub: Verify and delete released label
release-latest-tag-->>Maintainer: Report carry-forward and retirement result
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None 1 optional E2E recommendation
Blockers
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 @.agents/skills/nemoclaw-maintainer-cut-release-tag/SKILL.md:
- Around line 161-166: Update the workflow-monitoring instructions after Step 3
to identify the release run using the release commit and push event, rather than
selecting the most recent run. Filter the gh run list result by the release
commit, require exactly one matching run, and only then pass its run ID to gh
run watch.
In `@scripts/retire-release-label.mts`:
- Around line 43-48: Before calling ensureReleaseLabel in the retirement flow,
add an exact remote-tag check for to and refuse to recreate or reuse the label
when that version has already shipped. Keep the existing releaseLabelExists
handling and successful retirement output unchanged.
In `@test/maintainer-skills-policy.test.ts`:
- Line 101: Remove the duplicate quality_rules property declaration from the
relevant type definition in maintainer-skills-policy.test.ts, keeping a single
declaration with its existing boolean shape so the TypeScript
duplicate-identifier error is resolved.
🪄 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: Enterprise
Run ID: 7680ad28-1522-4e26-8d93-4fcfe41cf896
📒 Files selected for processing (21)
.agents/skills/nemoclaw-maintainer-cut-release-tag/SKILL.md.agents/skills/nemoclaw-maintainer-day/PR-REVIEW-PRIORITIES.md.agents/skills/nemoclaw-maintainer-day/scripts/bump-stragglers.ts.agents/skills/nemoclaw-maintainer-evening/SKILL.md.agents/skills/nemoclaw-maintainer-policies/SKILL.md.agents/skills/nemoclaw-maintainer-policies/references/daily-flow.md.agents/skills/nemoclaw-maintainer-policies/references/examples.md.agents/skills/nemoclaw-maintainer-policies/references/label-taxonomy.json.agents/skills/nemoclaw-maintainer-policies/references/label-taxonomy.md.agents/skills/nemoclaw-maintainer-policies/references/project-workflow.md.agents/skills/nemoclaw-maintainer-policies/references/release-train.md.agents/skills/nemoclaw-skills-guide/SKILL.md.github/workflows/label-merged-pr-release-target.yaml.github/workflows/release-latest-tag.yamlscripts/release-plan.mtsscripts/retire-release-label.mtstest/bump-stragglers.test.tstest/label-merged-pr-release-target-workflow.test.tstest/maintainer-skills-policy.test.tstest/release-latest-tag.test.tstest/retire-release-label.test.ts
💤 Files with no reviewable changes (2)
- .agents/skills/nemoclaw-maintainer-day/scripts/bump-stragglers.ts
- test/bump-stragglers.test.ts
| release: { application_policy: string; positive_signals: string[] }; | ||
| }; | ||
| quality_rules: { post_merge_release_labeling_allowed: boolean }; | ||
| quality_rules: { post_merge_untagged_release_labeling_allowed: boolean }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Remove the duplicate quality_rules declaration.
Line 101 declares the same property twice, causing a TypeScript duplicate-identifier error.
Proposed fix
- quality_rules: { post_merge_untagged_release_labeling_allowed: boolean };
quality_rules: { post_merge_untagged_release_labeling_allowed: boolean };🤖 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/maintainer-skills-policy.test.ts` at line 101, Remove the duplicate
quality_rules property declaration from the relevant type definition in
maintainer-skills-policy.test.ts, keeping a single declaration with its existing
boolean shape so the TypeScript duplicate-identifier error is resolved.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/retire-release-label.test.ts (1)
1-31: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftImport the CLI source from this root-level test.
runRetireReleaseLabelexecutesscripts/retire-release-label.mtsin a child process. The repository rule fortest/**/*.test.{ts,js}requires root-level tests to import CLI source; expose a testable entry point and inject theghdependency, or relocate true process/package-contract coverage to the designated package-contract area.As per path instructions, root-level tests must import CLI source; compiled-artifact assertions belong under
test/package-contract/.🤖 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/retire-release-label.test.ts` around lines 1 - 31, Replace the child-process execution in runRetireReleaseLabel with a direct import of the CLI source and a testable entry point that accepts an injected gh dependency. Keep genuine process or package-contract assertions out of this root-level test; if needed, move them under test/package-contract/.Source: Path instructions
🤖 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.
Outside diff comments:
In `@test/retire-release-label.test.ts`:
- Around line 1-31: Replace the child-process execution in runRetireReleaseLabel
with a direct import of the CLI source and a testable entry point that accepts
an injected gh dependency. Keep genuine process or package-contract assertions
out of this root-level test; if needed, move them under test/package-contract/.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4afeeae3-dce6-4c95-8d22-59648ee18cfb
📒 Files selected for processing (4)
.agents/skills/nemoclaw-maintainer-cut-release-tag/SKILL.mdscripts/retire-release-label.mtstest/maintainer-skills-policy.test.tstest/retire-release-label.test.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- test/maintainer-skills-policy.test.ts
- scripts/retire-release-label.mts
- .agents/skills/nemoclaw-maintainer-cut-release-tag/SKILL.md
## Summary - Requires release operators to create the immutable semver tag as a signed annotated tag on their workstation. - Requires GitHub to report that exact tag object as `verified: true` before the release workflow can promote `latest`. - Makes `latest` a second ref to the exact verified semver tag object instead of minting a distinct unsigned tag object in Actions. - Verifies raw tag-object equality as well as peeled commit equality, while retaining main ancestry, newest-semver, rollback, and unchanged-`lkg` guards. - Uses an exact-object force-with-lease so a concurrent out-of-band `latest` update cannot be overwritten. ## Why The current release workflow creates `latest` with `git tag -fa`, which produces a separate unsigned annotated object even when the semver release tag is signed. Authentication through `GITHUB_TOKEN` authorizes the ref update but does not cryptographically sign the object. This caused `latest` for v0.0.86 to report `verified: false` until protected-tag remediation. The new trust chain keeps private signing material out of Actions: the operator signs the immutable semver object locally, GitHub verifies it, and Actions only aliases the mutable `latest` ref to that already-verified object. ## Security Boundary This change authenticates the exact tag object and requires GitHub to recognize its signature. It does not introduce a separate allowlist of release signers: authorization to create a release tag remains governed by repository permissions and tag rulesets. Restricting tag creation to a designated release team or app, if required, is a separate policy hardening step. ## Coordination [#7118](#7118) also updates the latest-tag workflow for release-label retirement. This patch applies cleanly over its current head; if #7118 lands first, its permissions, concurrency group, and retirement steps must be preserved when this branch is refreshed. ## Type of Change - [x] Code change with tests - [x] Release/security workflow hardening ## Verification - `vitest run --project integration test/release-latest-tag.test.ts test/release-latest-tag-workflow.test.ts` — **22 passed** - Real ephemeral SSH signing keys prove the cut tag contains an SSH signature. - Git semantics test proves `latest` and `vX.Y.Z` resolve through the same signed tag object. - Negative tests cover mismatched verified object, permanently unverified GitHub state, verification propagation, distinct same-commit objects, and concurrent `latest` movement. - `npm run build:cli && npm run check:diff` — passed - `bash -n`, ShellCheck, formatting, repository checks, source-shape budget, test-size budget, gitleaks, and DCO hooks — passed - Commit is SSH-signed and includes DCO sign-off. --- Signed-off-by: Aaron Erickson <aerickson@nvidia.com> Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/release-latest-tag.test.ts (1)
223-229: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAssert that carry-forward precedes label deletion.
These assertions only verify that both operations exist. A plan that deletes
v0.0.2before carrying open items tov0.0.3would still pass. Assert their indices or ordered subsequence.Suggested assertion
- expect(plan.operations).toContain( - "have release-latest-tag workflow carry open v0.0.2 items forward to v0.0.3", - ); - expect(plan.operations).toContain( - "have release-latest-tag workflow delete released v0.0.2 label after carry-forward succeeds", - ); + const carryForward = + "have release-latest-tag workflow carry open v0.0.2 items forward to v0.0.3"; + const deleteReleased = + "have release-latest-tag workflow delete released v0.0.2 label after carry-forward succeeds"; + const carryForwardIndex = plan.operations.indexOf(carryForward); + const deleteReleasedIndex = plan.operations.indexOf(deleteReleased); + expect(carryForwardIndex).toBeGreaterThanOrEqual(0); + expect(deleteReleasedIndex).toBeGreater(carryForwardIndex);As per path instructions, this test should verify behavioral confidence at the public boundary rather than only operation presence.
🤖 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-latest-tag.test.ts` around lines 223 - 229, Update the release-latest-tag plan test around the carry-forward and label-deletion operation assertions to verify ordering, not just presence. Capture or derive each operation’s index in plan.operations and assert that carrying open v0.0.2 items forward to v0.0.3 occurs before deleting the released v0.0.2 label, while preserving the existing operation-content checks.Source: Path instructions
🤖 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.
Outside diff comments:
In `@test/release-latest-tag.test.ts`:
- Around line 223-229: Update the release-latest-tag plan test around the
carry-forward and label-deletion operation assertions to verify ordering, not
just presence. Capture or derive each operation’s index in plan.operations and
assert that carrying open v0.0.2 items forward to v0.0.3 occurs before deleting
the released v0.0.2 label, while preserving the existing operation-content
checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: f4718fa6-e285-4403-82e1-b0e0ad327163
📒 Files selected for processing (6)
.agents/skills/nemoclaw-maintainer-cut-release-tag/SKILL.md.github/workflows/release-latest-tag.yamlscripts/release-plan.mtstest/label-merged-pr-release-target-workflow.test.tstest/maintainer-skills-policy.test.tstest/release-latest-tag.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- .github/workflows/release-latest-tag.yaml
- scripts/release-plan.mts
- .agents/skills/nemoclaw-maintainer-cut-release-tag/SKILL.md
- test/label-merged-pr-release-target-workflow.test.ts
Summary
Release target labels are now temporary planning state instead of a second release-membership record. Merged PR automation labels only work ahead of the latest semver tag, and the coordinated tag workflow carries open work forward before deleting the released label; tags and commit ancestry remain the durable signal.
Changes
latest-tag..main.release-latest-tagworkflow run the idempotent retirement helper after verifying the signed release tag and movinglatestto that exact tag object; it moves open issues and PRs to the next patch, verifies none remain, and deletes the released repository label.queue: maxconcurrency group, preventing assignment during the final verification/delete window.Type of Change
Quality Gates
.agents/skills/; it does not change end-user behavior or Fern documentation.Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm run typecheck:cli, andnpm run test:projects:check(1,654 candidate files across 8 projects)npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
release-latest-tagflow verifies completion and then retires (deletes) the released label, using serialized concurrency and updated token permissions; retirement is restricted to this workflow.Documentation
Tests