fix(pr-risk): retire default labels after remap - #253
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Team Run ID: 📒 Files selected for processing (5)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe risk-label script now owns the five default risk labels and removes them after a custom ChangesRisk label remapping
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The change remaps risk labels while retiring the old defaults and documents the caller mapping; no actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
huang47
changed the title
|
close, just do caller side override Comfy-Org/ComfyUI_frontend#16463 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 6 finding(s).
| Severity | Count |
|---|---|
| 🟡 Medium | 1 |
| 🟢 Low | 3 |
| ⚪ Nit | 2 |
Panel: 6/6 reviewers contributed findings.
| [ -n "$l" ] || die "LABEL_MAP maps tier '$t' to an empty label" | ||
| OWNED+=("$l") | ||
| done | ||
| OWNED+=("risk:R0" "risk:R1" "risk:R2" "risk:R3" "risk:ungraded") |
There was a problem hiding this comment.
🟡 Medium — The five default names are appended to OWNED unconditionally and permanently, so in a repo enrolled with a custom label_map a risk:R0..risk:R3/risk:ungraded label this grader never applied — a human triage marker, an org escalation convention, a parallel/legacy labeler — is now silently deleted by the full-set PUT, which breaks the script's own promise that a human's own label is never fought. There is no caller opt-out, and if another actor re-adds one the PR can never satisfy the in-sync short-circuit, so every subsequent run flaps and re-opens the destructive read→PUT window the header claims is confined to grade-changing runs. Consider gating retirement behind an explicit one-time migration input instead of claiming unrelated labels by name.
Raised by 5 of 6 reviewers (claude-opus-5-thinking-max adversarial, gpt-5.6-sol-max adversarial, kimi-k3-high adversarial, claude-opus-5-thinking-max edge-case, kimi-k3-high edge-case).
| [ -n "$l" ] || die "LABEL_MAP maps tier '$t' to an empty label" | ||
| OWNED+=("$l") | ||
| done | ||
| OWNED+=("risk:R0" "risk:R1" "risk:R2" "risk:R3" "risk:ungraded") |
There was a problem hiding this comment.
🟢 Low — This list hardcodes the values already held in DEFAULT_MAP (line 79), so the two can silently desync: rename a default (say unknown → risk:unlabeled) and the new name is never retired while the stale one stays permanently owned in every repo, with nothing failing to signal it. Deriving the list from DEFAULT_MAP via the existing label_for logic keeps them identical by construction.
Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| # they are carried through every future PUT beside the new target, and nothing here will clean them | ||
| # up. Remapping is a one-time repo-side cleanup (delete the retired label names), not something a | ||
| # re-grade heals. | ||
| # ONE MORE LIMIT, on a different axis: ownership is defined by the CURRENT LABEL_MAP plus the five |
There was a problem hiding this comment.
🟢 Low — This rewritten paragraph contradicts the OWNERSHIP CONTRACT at the top of the same file (line 5 still says the script owns "EXACTLY the label names in LABEL_MAP's values", with non-owned labels carried through untouched). That contract is the stated safety justification for the destructive whole-set PUT, so line 5 needs the same "plus the five default names" qualifier or the next maintainer reasons about this write from an invariant that no longer holds.
Raised by 3 of 6 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case, kimi-k3-high edge-case).
|
|
||
| To show `risk:low` through `risk:xhigh`, pass this `label_map` in the caller: | ||
| `R0=risk:low,R1=risk:medium,R2=risk:high,R3=risk:xhigh,unknown=risk:unknown`. | ||
| The next grade removes a default `risk:R0`..`risk:R3` or `risk:ungraded` label |
There was a problem hiding this comment.
🟢 Low — "The next grade removes a default ... label" reads as one-time migration cleanup, but the code makes those five names grader-owned permanently: after adopting a custom map a caller can never keep risk:R0..risk:R3 or risk:ungraded on a PR for any other purpose, since every grade strips them again. Say that outright here, and in the same-understated wording at .github/workflows/pr-risk.yml:117 and scripts/pr-risk/README.md:201.
Raised by 2 of 6 reviewers (claude-opus-5-thinking-max adversarial, claude-opus-5-thinking-max edge-case).
| # re-dispatch on `pr_number` if a final grade looks wrong). It can no longer leave two contradictory | ||
| # labels on a PR under one `label_map`; remapping `label_map` orphans the old names, which is a | ||
| # one-time repo-side cleanup. What the shape does cost is a narrower residual: the PUT is built from | ||
| # labels on a PR under one `label_map`; the first remapped grade retires the default |
There was a problem hiding this comment.
⚪ Nit — "the first remapped grade retires the default risk:R0..risk:R3 and risk:ungraded labels" overstates two ways: a remap that keeps a default name as a live target (the 1-indexed scheme documented in this same file keeps unknown=risk:ungraded) does not retire that name, and retirement happens per-PR on that PR's next grade, not globally on the first remapped grade.
Raised by 1 of 6 reviewers (kimi-k3-high edge-case).
| [ -n "$l" ] || die "LABEL_MAP maps tier '$t' to an empty label" | ||
| OWNED+=("$l") | ||
| done | ||
| OWNED+=("risk:R0" "risk:R1" "risk:R2" "risk:R3" "risk:ungraded") |
There was a problem hiding this comment.
⚪ Nit — The append does not dedup, so under the default map — the common case, where these names are already in OWNED from the map — each is held twice. Set semantics downstream are unaffected, but the in-sync loop spawns up to ten has() jq subprocesses per PR instead of five and the DRY_RUN log prints a visibly duplicated owned set.
Raised by 1 of 6 reviewers (claude-opus-5-thinking-max edge-case).
2 participants
Summary:
label_mapinput; add no new configuration surfacerisk:R0throughrisk:R3andrisk:ungradedlabels on the first remapped grade so a PR keeps one grader-owned labelCaller mapping:
R0=risk:low,R1=risk:medium,R2=risk:high,R3=risk:xhigh,unknown=risk:unknownValidation: