diff --git a/docs/label-taxonomy.md b/docs/label-taxonomy.md index d1c400d..fd23d9f 100644 --- a/docs/label-taxonomy.md +++ b/docs/label-taxonomy.md @@ -39,6 +39,7 @@ one. | `status:blocked` | Issue is blocked | | `status:needs-review` | Issue work needs review | | `status:pr-open` | A linked PR is open | +| `status:changes-requested` | Reviewer requested changes on the linked PR | | `status:approved` | Linked PR is approved | | `status:merged` | Linked PR is merged | | `status:reconcile` | Issue needs reconciliation | @@ -46,6 +47,72 @@ one. | `status:duplicate` | Issue is a duplicate | | `status:wontfix` | Issue will not be fixed | +## Role Ownership Labels (#603) + +A single `role:*` label shows which workflow role currently owns the item. It is +advisory visibility only — the control-plane lease (#601) is the source of truth +for mutation authority. Only one `role:*` label is active at a time; tooling +replaces it on handoff via `transition_role_labels`. + +| Label | Use | +| --- | --- | +| `role:author` | Author currently owns the item | +| `role:reviewer` | Reviewer currently owns the item | +| `role:merger` | Merger currently owns the item | + +## Hazard Labels (#603) + +Hazard labels are orthogonal warning flags. Unlike `status:*` and `role:*`, +**more than one hazard may be active at once**, and a hazard never substitutes +for a live lease / PR-state check. Add/remove with `add_hazard_label` / +`clear_hazard_label`. + +| Label | Use | +| --- | --- | +| `hazard:stale-lease` | A stale or expired lease references this item | +| `hazard:workflow-contaminated` | Session/workflow state is contaminated; do not mutate | +| `hazard:conflicted` | Linked PR has merge conflicts | +| `hazard:root-mutation` | Work was mutated in the project root checkout | +| `hazard:manual-state` | Session or lease state was edited manually | +| `hazard:terminal-blocker` | A terminal review/merge lock blocks progress (#332/#602) | + +Any item that carries `status:blocked` or any `hazard:*` flag must also have a +blocking-reason / next-action comment (`requires_blocking_reason`). + +## `state:*` → canonical mapping (#603 migration) + +Issue #603 proposed a parallel `state:*` vocabulary. To avoid a conflicting +second lifecycle prefix, those requested states are folded into the existing +canonical labels rather than introduced as `state:*`. `state:*` is **not** a +supported prefix; use the canonical label on the right. + +| Requested `state:*` | Canonical label | +| --- | --- | +| `state:needs-triage` | `status:triage` | +| `state:claimed` | `status:claimed` | +| `state:authoring` | `status:in-progress` | +| `state:needs-review` | `status:needs-review` | +| `state:reviewing` | `status:needs-review` | +| `state:changes-requested` | `status:changes-requested` | +| `state:approved` | `status:approved` | +| `state:merge-ready` | `status:approved` | +| `state:merged` | `status:merged` | +| `state:blocked` | `status:blocked` | +| `state:terminal-blocker` | `hazard:terminal-blocker` | +| `state:abandoned` | `status:wontfix` | + +The transition helpers accept these names as synonyms (e.g. +`canonical_status_label("authoring")` → `status:in-progress`), so callers may use +the #603 wording while a single canonical status stays active. + +## Allocator Cross-Check (#603) + +Labels are advisory queue hints. The work allocator (#600/#613) uses labels as +one signal but **cross-checks live leases and PR state** and never trusts labels +alone. Discussion issues (`type:discussion`) are excluded from implementation +queues (`is_implementation_candidate`) unless a controller explicitly selects +them. + ## Transition Rules Suggested lifecycle: diff --git a/issue_workflow_labels.py b/issue_workflow_labels.py index 779834c..00028ca 100644 --- a/issue_workflow_labels.py +++ b/issue_workflow_labels.py @@ -34,6 +34,11 @@ STATUS_LABEL_SPECS: tuple[LabelSpec, ...] = ( LabelSpec("status:blocked", "b60205", "Issue is blocked"), LabelSpec("status:needs-review", "0052cc", "Issue work needs review"), LabelSpec("status:pr-open", "1d76db", "A linked PR is open"), + LabelSpec( + "status:changes-requested", + "e11d21", + "Reviewer requested changes on the linked PR", + ), LabelSpec("status:approved", "0e8a16", "Linked PR is approved"), LabelSpec("status:merged", "5319e7", "Linked PR is merged"), LabelSpec("status:reconcile", "d93f0b", "Issue needs reconciliation"), @@ -65,8 +70,42 @@ VALIDATION_LABEL_SPECS: tuple[LabelSpec, ...] = ( ), ) +# Lifecycle role-ownership labels (#603): which workflow role currently owns the +# item. Advisory visibility only — the control-plane lease (#601) remains the +# source of truth for mutation authority. Only one role:* label is active at a +# time, mirroring the single-active-status invariant. +ROLE_LABEL_SPECS: tuple[LabelSpec, ...] = ( + LabelSpec("role:author", "1d76db", "Author currently owns the item"), + LabelSpec("role:reviewer", "5319e7", "Reviewer currently owns the item"), + LabelSpec("role:merger", "0e8a16", "Merger currently owns the item"), +) + +# Lifecycle hazard labels (#603): orthogonal warning flags surfacing dangerous +# coordination conditions. Unlike status/role, multiple hazard:* labels may be +# active at once, and they never substitute for live lease / PR state checks. +HAZARD_LABEL_SPECS: tuple[LabelSpec, ...] = ( + LabelSpec("hazard:stale-lease", "d93f0b", "A stale or expired lease references this item"), + LabelSpec( + "hazard:workflow-contaminated", + "b60205", + "Session/workflow state is contaminated and must not mutate", + ), + LabelSpec("hazard:conflicted", "e11d21", "Linked PR has merge conflicts"), + LabelSpec("hazard:root-mutation", "b60205", "Work was mutated in the project root checkout"), + LabelSpec("hazard:manual-state", "d93f0b", "Session or lease state was edited manually"), + LabelSpec( + "hazard:terminal-blocker", + "000000", + "A terminal review/merge lock blocks progress (#332/#602)", + ), +) + CANONICAL_LABEL_SPECS: tuple[LabelSpec, ...] = ( - TYPE_LABEL_SPECS + STATUS_LABEL_SPECS + VALIDATION_LABEL_SPECS + TYPE_LABEL_SPECS + + STATUS_LABEL_SPECS + + VALIDATION_LABEL_SPECS + + ROLE_LABEL_SPECS + + HAZARD_LABEL_SPECS ) TYPE_LABELS: frozenset[str] = frozenset(spec.name for spec in TYPE_LABEL_SPECS) @@ -74,6 +113,8 @@ STATUS_LABELS: frozenset[str] = frozenset(spec.name for spec in STATUS_LABEL_SPE VALIDATION_LABELS: frozenset[str] = frozenset( spec.name for spec in VALIDATION_LABEL_SPECS ) +ROLE_LABELS: frozenset[str] = frozenset(spec.name for spec in ROLE_LABEL_SPECS) +HAZARD_LABELS: frozenset[str] = frozenset(spec.name for spec in HAZARD_LABEL_SPECS) CANONICAL_LABELS: frozenset[str] = frozenset( spec.name for spec in CANONICAL_LABEL_SPECS ) @@ -91,9 +132,15 @@ STATUS_TRANSITIONS: dict[str, str] = { "blocked": "status:blocked", "needs_review": "status:needs-review", "needs-review": "status:needs-review", + "reviewing": "status:needs-review", "pr_open": "status:pr-open", "pr-open": "status:pr-open", + "changes_requested": "status:changes-requested", + "changes-requested": "status:changes-requested", + "changes": "status:changes-requested", "approved": "status:approved", + "merge_ready": "status:approved", + "merge-ready": "status:approved", "merge": "status:reconcile", "merged": "status:reconcile", "reconcile": "status:reconcile", @@ -101,6 +148,37 @@ STATUS_TRANSITIONS: dict[str, str] = { "complete": "status:done", "duplicate": "status:duplicate", "wontfix": "status:wontfix", + "abandoned": "status:wontfix", + # #603 requested state:* synonyms folded into the canonical status vocabulary + "needs_triage": "status:triage", + "needs-triage": "status:triage", + "authoring": "status:in-progress", +} + +# #603: single-active role ownership. Maps role kinds / role:* labels to the +# canonical role label. Mirrors STATUS_TRANSITIONS for the role dimension. +ROLE_TRANSITIONS: dict[str, str] = { + "author": "role:author", + "reviewer": "role:reviewer", + "merger": "role:merger", +} + +# #603: hazard flag synonyms. Hazards are additive (not single-active), so this +# only normalizes names; it does not drive replacement. +HAZARD_TRANSITIONS: dict[str, str] = { + "stale_lease": "hazard:stale-lease", + "stale-lease": "hazard:stale-lease", + "workflow_contaminated": "hazard:workflow-contaminated", + "workflow-contaminated": "hazard:workflow-contaminated", + "contaminated": "hazard:workflow-contaminated", + "conflicted": "hazard:conflicted", + "conflict": "hazard:conflicted", + "root_mutation": "hazard:root-mutation", + "root-mutation": "hazard:root-mutation", + "manual_state": "hazard:manual-state", + "manual-state": "hazard:manual-state", + "terminal_blocker": "hazard:terminal-blocker", + "terminal-blocker": "hazard:terminal-blocker", } @@ -155,6 +233,100 @@ def transition_status_labels( return kept +def role_labels(labels: Iterable[str | Mapping[str, object]] | Mapping[str, object]) -> list[str]: + return [name for name in label_names(labels) if name.startswith("role:")] + + +def hazard_labels(labels: Iterable[str | Mapping[str, object]] | Mapping[str, object]) -> list[str]: + return [name for name in label_names(labels) if name.startswith("hazard:")] + + +def canonical_role_label(role_or_transition: str) -> str: + role = role_or_transition.strip() + if role in ROLE_LABELS: + return role + normalized = role.lower().replace(" ", "-") + try: + return ROLE_TRANSITIONS[normalized] + except KeyError as exc: + raise ValueError( + f"unknown workflow role or transition '{role_or_transition}'" + ) from exc + + +def transition_role_labels( + existing_labels: Iterable[str | Mapping[str, object]] | Mapping[str, object], + role_or_transition: str, +) -> list[str]: + """Replace all active role labels with the requested canonical role.""" + new_role = canonical_role_label(role_or_transition) + kept = [name for name in label_names(existing_labels) if not name.startswith("role:")] + if new_role not in kept: + kept.append(new_role) + return kept + + +def canonical_hazard_label(hazard: str) -> str: + name = hazard.strip() + if name in HAZARD_LABELS: + return name + normalized = name.lower().replace(" ", "-") + try: + return HAZARD_TRANSITIONS[normalized] + except KeyError as exc: + raise ValueError(f"unknown workflow hazard '{hazard}'") from exc + + +def add_hazard_label( + existing_labels: Iterable[str | Mapping[str, object]] | Mapping[str, object], + hazard: str, +) -> list[str]: + """Add a hazard flag without disturbing status/role/type labels (additive).""" + new_hazard = canonical_hazard_label(hazard) + kept = label_names(existing_labels) + if new_hazard not in kept: + kept.append(new_hazard) + return kept + + +def clear_hazard_label( + existing_labels: Iterable[str | Mapping[str, object]] | Mapping[str, object], + hazard: str, +) -> list[str]: + """Remove a single hazard flag, leaving all other labels intact.""" + target = canonical_hazard_label(hazard) + return [name for name in label_names(existing_labels) if name != target] + + +def is_discussion(labels: Iterable[str | Mapping[str, object]] | Mapping[str, object]) -> bool: + return "type:discussion" in type_labels(labels) + + +def is_implementation_candidate( + labels: Iterable[str | Mapping[str, object]] | Mapping[str, object], +) -> bool: + """Whether an item may enter an implementation queue on labels alone. + + Discussion issues are excluded (#603 AC3) unless a controller explicitly + selects them; the allocator still cross-checks live lease/PR state and never + trusts labels alone (#603 AC2). + """ + return not is_discussion(labels) + + +def requires_blocking_reason( + labels: Iterable[str | Mapping[str, object]] | Mapping[str, object], +) -> bool: + """Whether the item must carry a blocking-reason / next-action comment (AC4). + + True when blocked or when any hazard flag is present. + """ + names = label_names(labels) + if "status:blocked" in names: + return True + return any(name.startswith("hazard:") for name in names) + + def labels_for_new_issue( issue_type: str | None = None, initial_status: str | None = None, @@ -188,6 +360,8 @@ def assess_issue_labels( names = label_names(labels) found_types = type_labels(names) found_statuses = status_labels(names) + found_roles = role_labels(names) + found_hazards = hazard_labels(names) errors: list[str] = [] warnings: list[str] = [] @@ -202,6 +376,10 @@ def assess_issue_labels( "issue has multiple active status:* labels: " + ", ".join(found_statuses) ) + if len(found_roles) > 1: + errors.append( + "issue has multiple active role:* labels: " + ", ".join(found_roles) + ) for name in found_types: if name not in TYPE_LABELS: @@ -209,12 +387,20 @@ def assess_issue_labels( for name in found_statuses: if name not in STATUS_LABELS: warnings.append(f"unknown status label '{name}'") + for name in found_roles: + if name not in ROLE_LABELS: + warnings.append(f"unknown role label '{name}'") + for name in found_hazards: + if name not in HAZARD_LABELS: + warnings.append(f"unknown hazard label '{name}'") return { "valid": not errors, "labels": names, "type_labels": found_types, "status_labels": found_statuses, + "role_labels": found_roles, + "hazard_labels": found_hazards, "errors": errors, "warnings": warnings, } diff --git a/tests/test_issue_workflow_labels.py b/tests/test_issue_workflow_labels.py index e7fd143..c2fe94e 100644 --- a/tests/test_issue_workflow_labels.py +++ b/tests/test_issue_workflow_labels.py @@ -69,5 +69,139 @@ class TestIssueWorkflowStatusTransitions(unittest.TestCase): self.assertEqual(result, ["type:process", "status:duplicate"]) +class TestLifecycleRoleLabels(unittest.TestCase): + def test_role_transition_author_to_reviewer_to_merger_single_active(self): + after_author = labels.transition_role_labels( + ["type:feature", "status:pr-open"], "author" + ) + self.assertEqual(after_author, ["type:feature", "status:pr-open", "role:author"]) + + after_reviewer = labels.transition_role_labels(after_author, "reviewer") + self.assertEqual( + after_reviewer, ["type:feature", "status:pr-open", "role:reviewer"] + ) + self.assertNotIn("role:author", after_reviewer) + + after_merger = labels.transition_role_labels(after_reviewer, "merger") + self.assertEqual(labels.role_labels(after_merger), ["role:merger"]) + + def test_role_transition_accepts_canonical_label(self): + result = labels.transition_role_labels(["type:bug"], "role:reviewer") + self.assertEqual(result, ["type:bug", "role:reviewer"]) + + def test_unknown_role_raises(self): + with self.assertRaises(ValueError): + labels.canonical_role_label("wizard") + + def test_assess_reports_multiple_active_role_labels(self): + result = labels.assess_issue_labels( + ["type:feature", "status:pr-open", "role:author", "role:reviewer"] + ) + self.assertFalse(result["valid"]) + self.assertTrue( + any("multiple active role:* labels" in err for err in result["errors"]) + ) + self.assertEqual( + sorted(result["role_labels"]), ["role:author", "role:reviewer"] + ) + + +class TestLifecycleHazardLabels(unittest.TestCase): + def test_hazards_are_additive_and_multiple(self): + one = labels.add_hazard_label(["type:bug", "status:blocked"], "conflicted") + two = labels.add_hazard_label(one, "stale-lease") + self.assertIn("hazard:conflicted", two) + self.assertIn("hazard:stale-lease", two) + # status/type untouched + self.assertIn("status:blocked", two) + self.assertIn("type:bug", two) + self.assertEqual(len(labels.hazard_labels(two)), 2) + + def test_add_hazard_is_idempotent(self): + once = labels.add_hazard_label(["type:bug", "status:ready"], "root-mutation") + twice = labels.add_hazard_label(once, "root_mutation") + self.assertEqual(twice.count("hazard:root-mutation"), 1) + + def test_clear_hazard_leaves_others_intact(self): + start = ["type:bug", "status:blocked", "hazard:conflicted", "hazard:stale-lease"] + cleared = labels.clear_hazard_label(start, "conflicted") + self.assertNotIn("hazard:conflicted", cleared) + self.assertIn("hazard:stale-lease", cleared) + self.assertIn("status:blocked", cleared) + + def test_terminal_blocker_hazard_normalizes(self): + self.assertEqual( + labels.canonical_hazard_label("terminal-blocker"), + "hazard:terminal-blocker", + ) + + def test_assess_surfaces_hazard_labels(self): + result = labels.assess_issue_labels( + ["type:bug", "status:blocked", "hazard:conflicted"] + ) + self.assertEqual(result["hazard_labels"], ["hazard:conflicted"]) + + +class TestDiscussionExclusion(unittest.TestCase): + def test_discussion_issue_excluded_from_implementation_queue(self): + self.assertTrue(labels.is_discussion(["type:discussion", "status:ready"])) + self.assertFalse( + labels.is_implementation_candidate(["type:discussion", "status:ready"]) + ) + + def test_non_discussion_issue_is_candidate(self): + self.assertTrue( + labels.is_implementation_candidate(["type:feature", "status:ready"]) + ) + + +class TestBlockingReasonRequirement(unittest.TestCase): + def test_blocked_requires_reason(self): + self.assertTrue( + labels.requires_blocking_reason(["type:bug", "status:blocked"]) + ) + + def test_hazard_requires_reason(self): + self.assertTrue( + labels.requires_blocking_reason( + ["type:bug", "status:ready", "hazard:stale-lease"] + ) + ) + + def test_clean_ready_issue_needs_no_reason(self): + self.assertFalse( + labels.requires_blocking_reason(["type:feature", "status:ready"]) + ) + + +class TestState603SynonymTransitions(unittest.TestCase): + def test_authoring_maps_to_in_progress(self): + self.assertEqual( + labels.canonical_status_label("authoring"), "status:in-progress" + ) + + def test_changes_requested_transition(self): + result = labels.transition_status_labels( + ["type:feature", "status:needs-review"], "changes-requested" + ) + self.assertEqual(result, ["type:feature", "status:changes-requested"]) + + def test_merge_ready_maps_to_approved(self): + self.assertEqual(labels.canonical_status_label("merge-ready"), "status:approved") + + def test_abandoned_maps_to_wontfix(self): + self.assertEqual(labels.canonical_status_label("abandoned"), "status:wontfix") + + def test_blocked_and_merged_transitions(self): + blocked = labels.transition_status_labels( + ["type:bug", "status:in-progress"], "blocked" + ) + self.assertEqual(blocked, ["type:bug", "status:blocked"]) + merged = labels.transition_status_labels( + ["type:feature", "status:approved"], "merged" + ) + self.assertEqual(merged, ["type:feature", "status:reconcile"]) + + if __name__ == "__main__": unittest.main()