Repository navigation
UN-4522 [MISC] Patch a released line and add a late target in the release tooling - #2441
chandrasekharan-zipstack wants to merge 27 commits into
Conversation
Mirror the cloud release tooling: refusals now say a release manager may override with force, Cut accepts a minor whose line was abandoned, and the test for the retired cloud workflows is gone with them. Mark create-release as the public OSS release, separate from the ee- release-branch tags, and drop the "V2 " prefix from the release workflow names. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jfBJqWg2hcUEBx2rvJMga
Mirror of the release tooling change that pushes an on-prem RC chart with each on-prem RC. Chart versions carry no "v" prefix, so the RC pattern Abandon deletes by accepts both forms. The test for the RC build summary's chart result is mirrored too, so the release tooling stays identical between the two repositories. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jfBJqWg2hcUEBx2rvJMga
Mirror of the release tooling change in the cloud repository. Abandon sorts chart versions numerically, so it keeps rc.10 rather than rc.9, and RC allocation skips RC numbers that the on-prem RC chart already holds. The repository has no on-prem target, so the allocation change does nothing here; it keeps the release tooling identical in both repositories. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jfBJqWg2hcUEBx2rvJMga
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jfBJqWg2hcUEBx2rvJMga
Take RC build already skips them, so State showed an RC number the build would not use. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jfBJqWg2hcUEBx2rvJMga
A change to ci-test.yaml itself still runs them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jfBJqWg2hcUEBx2rvJMga
…paths Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jfBJqWg2hcUEBx2rvJMga
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jfBJqWg2hcUEBx2rvJMga
The release paths shared the `every` quantifier of the ignore filter, so no file could match them all and release-tests always skipped. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jfBJqWg2hcUEBx2rvJMga
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011jfBJqWg2hcUEBx2rvJMga
A target whose first RC comes after another target's GA on the line (on-prem enabled after cloud GA) no longer takes the line back to stabilizing. relstate derives "late" from registry push times, and the line state counts a late target with no GA as stabilized: Backport picks only RM-confirmed security/critical fixes, Retire does not list the line, and the OSS line keeps stabilized/X.Y. Take RC build drops the job that deleted that tag. The owed-picks table on a forced late target is information only and asks the RM to label any fix the line needs. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A target added after the line's first GA keeps the line stabilized until its own GA, so retiring that line early is held like a stabilized on-prem line: refused without force. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Brings relmgr.py and test_links.py in line with the enterprise repo, so .github/release is identical in both. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
With confirm_stabilized, a plain fix now goes onto the stabilized lines named in branches. With branches empty it skips them, and the summary says why. Security and critical routing and the push-to-main rescan are unchanged. Release state lists, per stabilized line, the main fixes merged since its latest GA that it lacks, ready to paste into Backport commits. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A plain fix can now reach a stabilized line through Backport with confirm_stabilized and branches, so the summary says how instead of asking for a security or critical label. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
On a rerun Take RC plans builds only for the targets an earlier run missed. If that earlier run failed before its prerelease job, the release the rerun creates listed only those targets. The plan step now also outputs the ticked targets, which are all built once the run succeeds, and the prerelease job lists those. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
relmgr.line normalises "0.186" or "release/0.186" to "0.186" and refuses anything else; relmgr.vtag adds the leading v. Backport's branches input goes through line, and its Next hints print X.Y. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It takes release lines X.Y; the CLI flag is --lines and the Next hints print -f lines=. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
- Release line inputs no longer strip whitespace and accept ASCII digits only, so Take RC's concurrency group (keyed on the raw input) cannot be split by a padded value. Backport `lines` splits on commas or spaces. - Retire's early-retirement refusal checks the cloud target is actually stabilized; a late cloud target with no GA needs force, like on-prem. - Backport: a plain fix naming a stabilized line without confirm_stabilized says so (warning) instead of "not a target". - State's patch list reads from each line's first GA, not its latest, so fixes merged before a later patch are not dropped. A line with no GA, or a gh/git failure on one line, is reported instead of failing the run. - Comments and docstrings follow the code. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
chandrasekharan-zipstack
left a comment
chandrasekharan-zipstack
left a comment
There was a problem hiding this comment.
Review: round 1 (review-deep, INITIAL) at be2c5ff30e9639cbf93e26b46d534034955c20bc
Self-review round 1, run as unstract:review-deep because review-shallow refuses this PR (it changes CI workflows). Diff: merge base a1940732b9c65007b77e712fcd376ab696ab7267 to be2c5ff30, 10 files, 1051 lines. The base was fetched fresh. Signals raised lenses 4, 8, 11, 16 and 17.
Verdict — REQUEST CHANGES
Summary — Critical: 0 · High: 0 · Medium: 5 · Low: 7 · Lenses run: 19/19 (5, 6, 12, 14, 18 and 19 are N/A, with reasons below)
Security deep-dive: done, 0 findings (no candidate reached confidence 8).
Boundary sweep: 6 outbound sinks traced (step summary, ::warning::/::error:: lines, PR comments, held notes, state.json, the run log). None carries a credential or token.
Findings
The 12 numbered findings are posted as inline comments below.
Unanchored findings
- The PR body says "The RC prerelease target list is fixed". In this repo that change only adds a test,
TakeRcPrerelease, which reads the enterprise workflow. The body should say so. - Lens 1: this repo states no PR-title convention. The title follows the workspace convention, and the type
[MISC]fits release tooling. - The public functions
oss.reopenandrelstate.reopensare removed. Neither repo has a caller left.
Lens checklist
- Correctness — Clean
- Readability & maintainability — Clean
- Error handling — See findings #1, #8
- Security — Clean (security deep-dive done, boundary sweep clean)
- Data integrity & migrations — N/A — no database, migration or persisted schema; state.json is regenerated on every run
- Concurrency — N/A — no new concurrency; the workflow concurrency groups are unchanged
- API compatibility — See findings #3, #4, #9, #10, #11 (consumer search done in both repos by the orchestrator)
- Reliability & resilience — Clean
- Performance — Clean
- Observability — Clean
- Operational safety — Clean
- LLM/agent — N/A — no prompt, model or agent configuration
- Tests — See findings #2, #5, #12
- Dependencies & build — N/A — no dependency or lockfile change
- Style & lint — Clean (pre-commit ruff and ruff-format passed at this commit)
- Doc accuracy — See findings #1, #6, #7
- Cross-document consistency — See findings #7
- On-prem experience — N/A — release tooling for the repo's own CI; nothing reaches a customer cluster
- Scaling & workload lifecycle — N/A — no worker, scaling or lifecycle configuration
I assessed these lenses myself: 4, 5, 6, 7 (the consumer search), 8, 11, 12, 14, 15, 17, 18 and 19.
Open questions for the author
None.
Assumptions made
- The paired enterprise PR merges together with this one, as the body says. Findings #3 and #4 rely on that.
cost: agents=7 wall=12:36 lenses-examined=13 of 19 files=10 lines=1051
Unanchored findings
.github/workflows/backport.yaml:8(outside the diff hunk): #7 [Low] [Lens 16, 17] — The OSS backport.yaml header is stale —.github/workflows/backport.yaml:L8(outside the diff hunk). It still says "Security/critical fixes reach stabilized lines only with confirm_stabilized". A plain fix now also reaches them, withconfirm_stabilizedandlines. Made stale by:.github/workflows/backport.yaml:L24.
| try: | ||
| rows = held( | ||
| slug, REPO, branch, accept=OWED, cache=cache, since=ga["pushed_at"] |
There was a problem hiding this comment.
#1 [Medium] [Lens 3, 16] — "Patch a released line" leaves out fixes still owed from before the line's first GA — .github/release/backport.py:L512-L514. held(..., since=ga["pushed_at"]) drops every main commit dated before the first GA. The comment's reason ("From then on Backport stops picking plain fixes") does not hold. A fix can still be owed from before the GA in three ways: its fix/* label was added after the GA; its push-run rescan ran after Promote; or its push run failed. The list the RM pastes from then never shows that fix. Take RC's Owed picks, which counts from the cut, does show it. The trailer filter already drops what was picked, so since only removes commits that are still owed. Fix: bound by the cut (drop since). Confidence High on the mechanism. Verification: traced through held(), rescan_targets and pr_classes.
| if [ "$GITHUB_EVENT_NAME" = push ]; then args=(--rescan); else args=(--commits="$COMMITS"); fi | ||
| if [ "$CONFIRM" = true ]; then args+=(--confirm-stabilized); fi | ||
| python3 "$RUNNER_TEMP/backport.py" --state "$RUNNER_TEMP/state.json" "${args[@]}" --branches="$BRANCHES" | ||
| python3 "$RUNNER_TEMP/backport.py" --state "$RUNNER_TEMP/state.json" "${args[@]}" --lines="$LINES" |
There was a problem hiding this comment.
#2 [Medium] [Lens 13] — Nothing tests the Pick step's flags and inputs against backport.py's argparse — .github/workflows/backport.yaml:L91. The rename (--branches/branches to --lines/lines) was done by hand on both sides. A later drift makes argparse exit 2 on every Backport run, including each push-to-main rescan, while the suite stays green. Fix: add a test that every flag the step passes is in backport.py --help, and that every inputs.X it reads is declared.
| default: false | ||
| branches: | ||
| description: "Only these branches (optional)" | ||
| lines: |
There was a problem hiding this comment.
#3 [Medium] [Lens 7] — Input branches is renamed to lines with no alias — .github/workflows/backport.yaml:L27. Saved -f branches= commands now fail with 422, including held notes posted by earlier runs. Neither repo has a caller that uses the old name. The rename is deliberate and the PR body states it under Rollout. No change.
| # needs force) until it has a GA. | ||
| row["state"] = _line_state( | ||
| { | ||
| "stabilized" if tt["late"] and not tt["ga"] else tt["state"] |
There was a problem hiding this comment.
#4 [Medium] [Lens 7] — state.json: a late target with no GA now makes the line stabilized — .github/release/relstate.py:L531. The line moves from backport_targets.fix to security_critical. This is intended and the PR body states it. Every consumer regenerates state.json in the same run, so a tt["late"] KeyError on an old state.json cannot happen. No change.
| self.assertIn("::warning::⚠ Could not list open backport conflict PRs", r.stdout) | ||
|
|
||
|
|
||
| class LateTargetWorkflow(Workflow): |
There was a problem hiding this comment.
#5 [Medium] [Lens 13] — The workflow-level tests are skipped in this repo — .github/release/test_oss.py:L2552. LateTargetWorkflow, InputSteps, CutPickLine, TakeRcPrerelease and TakeRcVersion need the enterprise workflows and skip here. They run in the paired enterprise PR against a byte-identical .github/release, and the PR body says so. No change.
| return v | ||
|
|
||
|
|
||
| def vtag(value): |
There was a problem hiding this comment.
#8 [Low] [Lens 3] — relmgr.vtag validates nothing — .github/release/relmgr.py:L43. relmgr.py vtag "" prints v and exits 0. Before this PR, Promote and Publish used the raw input with no check either, so no input does worse than it did. A bad tag still fails at Promote's RC lookup. No change.
| "", | ||
| "Backport commits: " | ||
| + " ".join(s for s, cls, *_ in rows if cls != UNCLASSIFIABLE), | ||
| f"To pick a fix: dispatch Backport with its commit, `confirm_stabilized=true` and `lines={rm.line(branch)}`" |
There was a problem hiding this comment.
#9 [Low] [Lens 7] — owed_summary drops its machine-pasteable "Backport commits:" line — .github/release/backport.py:L444. No caller outside the tests parses it. The paste list moved to State's "Patch a released line". No change.
| return f"stabilized (GA {tt['ga']['tag']})" | ||
| if tt["state"] == "stabilizing" and tt["rcs"]: | ||
| return f"stabilizing (rc.{VER_RE.fullmatch(tt['rcs'][-1])[4]})" | ||
| return f"stabilizing (rc.{VER_RE.fullmatch(tt['rcs'][-1])[4]}{', late' * tt['late']})" |
There was a problem hiding this comment.
#10 [Low] [Lens 7] — The summary cell gains a , late suffix — .github/release/relstate.py:L768. No parser of that cell exists. No change.
| targets = state["backport_targets"] | ||
| listed = [b.strip() for b in a.branches.split(",") if b.strip()] | ||
| try: | ||
| listed = listed_branches(a.lines) |
There was a problem hiding this comment.
#11 [Low] [Lens 7] — A malformed lines item now fails the run instead of skipping — .github/release/backport.py:L973. This is stricter validation and is intended. No change.
| env: | ||
| GH_TOKEN: ${{ github.token }} | ||
| # Into the summary from Python: held() prints its own progress to stdout. | ||
| run: python3 -c 'import json, os, backport; open(os.environ["GITHUB_STEP_SUMMARY"], "a").write(backport.patch_lines(json.load(open("../../state.json"))) + "\n")' |
There was a problem hiding this comment.
#12 [Low] [Lens 13] — The python3 -c one-liner in State's new step has no test — .github/workflows/state.yaml:L39. patch_lines itself is unit-tested and its mutants die. The wrapper's cwd and paths match the steps around it.
The State list bounded each line at its first GA, which dropped a fix still owed from before it: a label added after the GA, or a push run that failed or ran after Promote. The trailer filter already drops what was picked, so the cutoff only removed owed commits. The PR listing still starts at the oldest GA. Also adds a test that the Backport Pick step passes only flags and inputs that exist, and corrects the Backport workflow header: a plain fix also reaches a stabilized line. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every gh call Backport and its record job make now goes through one helper that retries a 5xx, a reply cut short, or a timeout, twice with a short wait. A 4xx is GitHub's answer and is never retried. The squash merge and PR create are writes that can land although gh reports a failure, so they are not retried blindly. A failed squash checks whether the PR merged before retrying or failing. A failed create looks for the PR first; when none exists after the retries, the run deletes the head it pushed, so the next run picks the commit again instead of finding an orphan head. The 405 "Base branch was modified" retry is unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A fix Backport failed to land on a stabilizing line used to show first at Promote's convergence gate, after QA had tested the RC. Release state now lists, per stabilizing line, the owed main fixes the line lacks, as Backport `commits`, and the open backport conflict PRs on it here and in the OSS repo. A clean line says none; a listing that fails says so rather than reading as clean. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Each release workflow's run summary now renders commits, release lines, tags and PRs as links to the repo they live in: OSS pins, OSS commits and cross-repo conflict PRs link to the OSS repo. Lines people paste into Backport (the commits lists and gh commands) stay plain. Retire links a branch only on a dry run and the archive tag only on a real run, since a real run deletes the one and creates the other. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
GitHub's commit-to-PR lookups can miss a squash merge, so Backport read
a merged fix as "no PR" and skipped it.
- fix_class: when commits/{sha}/pulls is empty, take the trailing (#N)
from the commit subject and accept that PR only if it is merged and
its merge_commit_sha equals the commit. A ::notice:: records the use.
- dispatch: a PR named by number is classed from the PR already
fetched, without the backward lookup.
- rescan: an unclassifiable commit fails the run with ::error:: until
it is picked or declined.
- dispatch: a named commit with no PR is a failed row, not "not a fix".
A PR whose class is not a fix stays a skip.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
 [Medium impact] The changes reach release managers and the Backport and State workflows for supported release lines. The PR appears safe to merge, with a non-blocking gap in release-branch test coverage.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Release workflow scripts. Unlike the tiers it also runs on drafts; report gates on it. | ||
| release-tests: | ||
| needs: changes | ||
| if: needs.changes.outputs.release == 'true' |
There was a problem hiding this comment.
Moving the release suites into ci-test.yaml drops their coverage for PRs targeting release/**. This workflow only starts for PRs targeting main, while the deleted workflow ran these suites for any PR that changed release tooling. Release-script fixes can now merge onto a release branch without those tests. Keep a release-branch trigger or a small separate workflow to preserve that coverage.
Prompt To Fix With AI
This is a comment left during a code review.
Path: .github/workflows/ci-test.yaml
Line: 299-302
Comment:
**Release branches lose test coverage**
Moving the release suites into `ci-test.yaml` drops their coverage for PRs targeting `release/**`. This workflow only starts for PRs targeting `main`, while the deleted workflow ran these suites for any PR that changed release tooling. Release-script fixes can now merge onto a release branch without those tests. Keep a release-branch trigger or a small separate workflow to preserve that coverage.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Unstract test resultsPer-group results
Critical paths
|
Stacked on #2431: review and merge that one first.
Why
This is the OSS half of the enterprise release-tooling change.
.github/releaseis shared with the enterprise repo and must stay byte-identical there.The enterprise change covers:
What
.github/releaseis synced with the enterprise copy:confirm_stabilizedputs a plain fix on the stabilized lines named inlines, and the owed-picks summary points there. Withoutconfirm_stabilized, a named stabilized line gets a warning row.relmgr.lineandrelmgr.vtaginput helpers;linerefuses padded values.backport.yaml:branchesis renamed tolinesand accepts0.186orrelease/0.186;--lines;confirm_stabilizedandlines.state.yaml: new "Patch a released line" step, which lists the fixes each stabilized line lacks since its cut. A failed lookup on one line is reported, not fatal.stabilized/X.Ywhen a late target is added.ghcall. A failed squash or PR create checks whether it landed before retrying, and a create that never landed deletes the head it pushed.ghcommands stay plain.(#N)from the subject and accepts that PR only if it is merged with that exact merge commit. An unclassifiable commit now fails a rescan, and a dispatched commit with no PR is a failed row.state.yaml: new "Missed picks on stabilizing lines" step, which lists per stabilizing line the owed fixes it lacks and the open conflict PRs on it in both repos.Testing
All six suites in
.github/releasepass:python3 test_*.py, one file at a time. Workflow tests that need the enterprise workflows are skipped here and run in the enterprise repo.Mutation checks, each mutant run against the full suite in the enterprise repo:
bash -nonly; Take RC build's fail headers and Prerelease line)Rollout
.github/releasemust stay byte-identical.ci-test.yaml; this branch merges it in.backport.yaml -f branches=...no longer works. Use-f lines=0.186,0.188.🤖 Generated with Claude Code