[PR #331] [MERGED] feat(area-label): deterministic path sub-labels alongside the classified area #332

New issue

Closed

opened 2026-10-26 22:38:36 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-26 22:38:36 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/Comfy-Org/github-workflows/pull/331
Author: @mattmillerai
Created: 9/19/2026
Status: ✅ Merged
Merged: 9/19/2026
Merged by: @mattmillerai

Base: main ← Head: matt/area-label-path-sub-labels

📝 Commits (1)

233dd8a feat(area-label): deterministic path sub-labels alongside the classified area

📊 Changes

10 files changed (+562 additions, -27 deletions)

View changed files

📝 .github/workflows/pr-area-label.yml (+11 -2)
📝 .github/workflows/test-area-label.yml (+11 -4)
📝 README.md (+1 -1)
📝 docs/callers/pr-area-label.md (+42 -1)
📝 scripts/area-label/README.md (+38 -4)
📝 scripts/area-label/classify-and-apply.sh (+56 -14)
📝 scripts/area-label/lib.sh (+102 -0)
📝 scripts/area-label/sync-labels.sh (+23 -1)
➕ scripts/area-label/tests/test_apply.sh (+200 -0)
📝 scripts/area-label/tests/test_lib.sh (+78 -0)

📄 Description

ELI-5

The area classifier picks exactly one area:* label and deletes every other one. That is the right shape for "which area owns this PR?" — but it leaves a subsystem that lives inside another area's service with no way to stay filterable. Both options are bad:

leave it under its parent's label → those PRs are invisible as a group;
promote it to its own area → those PRs now drop out of the parent's filter, which is exactly what the parent's reviewers want to keep seeing.

The fix is that it was never a one-or-the-other question. "Which area owns this?" is a judgement call, which is why a model makes it. "Does this PR touch X?" is a path question, and a path question has a deterministic answer that can ride alongside the classification instead of competing with it.

So: an optional sub_labels: block in the taxonomy. Each entry is an area:* name carrying paths: globs, matched against the PR's changed files — no model, no extra API call, no extra cost. A PR touching those paths ends up with the classified area and the sub-label.

What changed

scripts/area-label/lib.sh — four new pure functions, all hermetically tested:

taxonomy_sub_labels — parses the optional block; [] when absent.
validate_sub_labels — the gate. Names must be canonical area:* slugs, unique, disjoint from labels[], each with at least one non-blank glob.
glob_to_regex — segment-aware glob → anchored regex. */? stay within a segment, ** crosses them, a leading **/ matches zero or more directories. Deliberately the same semantics a workflow paths: filter uses, so a glob can be copied between the two. Everything else that means something to a regex is escaped.
matched_sub_labels — the names whose globs hit at least one changed file, in taxonomy order.

scripts/area-label/classify-and-apply.sh — the end state is now KEEP = the classified area plus every match. That one value drives all three decisions: the no-op short-circuit, the additive POST, and the cleanup's exemption list. The matched names are exempt from the area:* cleanup; a sub-label that stops matching is not in KEEP, so the same loop retires it automatically when a PR drops those files.

The sub-label POST is deliberately separate from the primary one, so the existing "add the classified area FIRST, so no failure can leave a PR unlabeled" guarantee is untouched — a hiccup adding a sub-label can never cost the PR its area.

scripts/area-label/sync-labels.sh — sub-labels are ordinary repository labels; only how they're applied differs. They're created and healed from the taxonomy exactly like the classified ones.

Why the names are constrained

^area:[a-z0-9-]+$ and disjoint from labels[] is load-bearing twice:

Every write this tool makes stays inside the area:* namespace. These names are handed to gh label create --force and to the label delete path, so an unconstrained name could overwrite or remove an unrelated repository label.
No label can be both "the classification" and "an exempt sub-label" — which would make the cleanup's "is this stale?" question ambiguous.

A malformed block skips the run with a warning rather than labeling under half a taxonomy, matching the existing validate_names / validate_vocab gates.

Opt-in and backwards compatible: a taxonomy without the key parses as an empty set and behaves exactly as before. No caller change is needed to adopt it.

Tests

scripts/area-label/tests/test_apply.sh is new. classify-and-apply.sh had zero coverage — the lib suite only ever exercised the pure core — and this change lands squarely in its label-writing path, which is the part that can do damage across every consuming repo.

gh and curl are stubbed, and the fake gh holds the PR's label set in a file that POST/DELETE actually mutate. That's what makes the assertions end states rather than a transcript diff:

a PR matching the globs gets both labels;
one that matches nothing gets only the classified area;
re-running on a correct PR is a no-op — not one API write;
a sub-label that no longer matches is retired;
a misclassified PR is corrected without losing its matched sub-label;
non-area: labels survive the cleanup untouched;
dry-run writes nothing but prints the full decision;
and the fail-soft paths (no API key, malformed sub_labels, HTTP 500) all write nothing.

tests/test_lib.sh gains 23 assertions over the new functions, including the glob semantics table and the disjointness gate.

CI now loops tests/*.sh instead of naming one file — bash a.sh b.sh runs only a.sh, so a listed second suite would have been silently skipped. This is the convention AGENTS.md already documents for shell suites.

Non-vacuity

Mutating matched_sub_labels to never match kills 9 assertions in test_apply.sh and 7 in test_lib.sh. Restored, both suites are green.

test_apply.sh: 20 passed, 0 failed
test_lib.sh:   55 passed, 0 failed
shellcheck -x scripts/area-label/*.sh scripts/area-label/tests/*.sh   # clean

Also green: check_workflow_pins.py, check-org-repo-literals.sh, check_agents_md.py.

A glob_to_regex bug worth noting, since shellcheck caught it and a test now pins it: a lone backslash written as '\\' in a case pattern is two characters and can never match a one-character input, so a literal \ in a glob would have gone unescaped into the regex.

Docs

scripts/area-label/README.md, docs/callers/pr-area-label.md (including the glob semantics and the honest limits — matching is paths-only, and the changed-file list comes from gh pr view --json files, which caps at 100), the README catalog row, and the reusable's header contract.

🤖 Generated with Claude Code

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/Comfy-Org/github-workflows/pull/331 **Author:** [@mattmillerai](https://github.com/mattmillerai) **Created:** 9/19/2026 **Status:** ✅ Merged **Merged:** 9/19/2026 **Merged by:** [@mattmillerai](https://github.com/mattmillerai) **Base:** `main` ← **Head:** `matt/area-label-path-sub-labels` --- ### 📝 Commits (1) - [`233dd8a`](https://github.com/Comfy-Org/github-workflows/commit/233dd8a49575271c2f9a01a659d3697492b1c583) feat(area-label): deterministic path sub-labels alongside the classified area ### 📊 Changes **10 files changed** (+562 additions, -27 deletions) <details> <summary>View changed files</summary> 📝 `.github/workflows/pr-area-label.yml` (+11 -2) 📝 `.github/workflows/test-area-label.yml` (+11 -4) 📝 `README.md` (+1 -1) 📝 `docs/callers/pr-area-label.md` (+42 -1) 📝 `scripts/area-label/README.md` (+38 -4) 📝 `scripts/area-label/classify-and-apply.sh` (+56 -14) 📝 `scripts/area-label/lib.sh` (+102 -0) 📝 `scripts/area-label/sync-labels.sh` (+23 -1) ➕ `scripts/area-label/tests/test_apply.sh` (+200 -0) 📝 `scripts/area-label/tests/test_lib.sh` (+78 -0) </details> ### 📄 Description ## ELI-5 The area classifier picks **exactly one** `area:*` label and deletes every other one. That is the right shape for "which area owns this PR?" — but it leaves a subsystem that lives *inside* another area's service with no way to stay filterable. Both options are bad: - leave it under its parent's label → those PRs are invisible as a group; - promote it to its own area → those PRs now drop **out** of the parent's filter, which is exactly what the parent's reviewers want to keep seeing. The fix is that it was never a one-or-the-other question. "Which area owns this?" is a judgement call, which is why a model makes it. "Does this PR touch X?" is a **path** question, and a path question has a deterministic answer that can ride *alongside* the classification instead of competing with it. So: an optional `sub_labels:` block in the taxonomy. Each entry is an `area:*` name carrying `paths:` globs, matched against the PR's changed files — no model, no extra API call, no extra cost. A PR touching those paths ends up with **the classified area *and* the sub-label**. ## What changed **`scripts/area-label/lib.sh`** — four new pure functions, all hermetically tested: - `taxonomy_sub_labels` — parses the optional block; `[]` when absent. - `validate_sub_labels` — the gate. Names must be canonical `area:*` slugs, unique, **disjoint from `labels[]`**, each with at least one non-blank glob. - `glob_to_regex` — segment-aware glob → anchored regex. `*`/`?` stay within a segment, `**` crosses them, a leading `**/` matches zero or more directories. Deliberately the same semantics a workflow `paths:` filter uses, so a glob can be copied between the two. Everything else that means something to a regex is escaped. - `matched_sub_labels` — the names whose globs hit at least one changed file, in taxonomy order. **`scripts/area-label/classify-and-apply.sh`** — the end state is now `KEEP` = the classified area **plus** every match. That one value drives all three decisions: the no-op short-circuit, the additive POST, and the cleanup's exemption list. The matched names are exempt from the `area:*` cleanup; a sub-label that *stops* matching is not in `KEEP`, so the same loop **retires it automatically** when a PR drops those files. The sub-label POST is deliberately separate from the primary one, so the existing "add the classified area FIRST, so no failure can leave a PR unlabeled" guarantee is untouched — a hiccup adding a sub-label can never cost the PR its area. **`scripts/area-label/sync-labels.sh`** — sub-labels are ordinary repository labels; only how they're *applied* differs. They're created and healed from the taxonomy exactly like the classified ones. ### Why the names are constrained `^area:[a-z0-9-]+$` and disjoint from `labels[]` is load-bearing twice: 1. **Every write this tool makes stays inside the `area:*` namespace.** These names are handed to `gh label create --force` and to the label delete path, so an unconstrained name could overwrite or remove an unrelated repository label. 2. **No label can be both "the classification" and "an exempt sub-label"** — which would make the cleanup's "is this stale?" question ambiguous. A malformed block **skips the run with a warning** rather than labeling under half a taxonomy, matching the existing `validate_names` / `validate_vocab` gates. Opt-in and backwards compatible: a taxonomy without the key parses as an empty set and behaves exactly as before. No caller change is needed to adopt it. ## Tests **`scripts/area-label/tests/test_apply.sh` is new.** `classify-and-apply.sh` had **zero** coverage — the lib suite only ever exercised the pure core — and this change lands squarely in its label-*writing* path, which is the part that can do damage across every consuming repo. `gh` and `curl` are stubbed, and the fake `gh` holds the PR's label set in a file that POST/DELETE actually mutate. That's what makes the assertions *end states* rather than a transcript diff: - a PR matching the globs gets **both** labels; - one that matches nothing gets only the classified area; - re-running on a correct PR is a **no-op** — not one API write; - a sub-label that no longer matches is **retired**; - a misclassified PR is corrected **without losing** its matched sub-label; - non-`area:` labels survive the cleanup untouched; - dry-run writes nothing but prints the full decision; - and the fail-soft paths (no API key, malformed `sub_labels`, HTTP 500) all write nothing. `tests/test_lib.sh` gains 23 assertions over the new functions, including the glob semantics table and the disjointness gate. **CI now loops `tests/*.sh`** instead of naming one file — `bash a.sh b.sh` runs only `a.sh`, so a listed second suite would have been silently skipped. This is the convention AGENTS.md already documents for shell suites. ## Non-vacuity Mutating `matched_sub_labels` to never match kills **9** assertions in `test_apply.sh` and **7** in `test_lib.sh`. Restored, both suites are green. ``` test_apply.sh: 20 passed, 0 failed test_lib.sh: 55 passed, 0 failed shellcheck -x scripts/area-label/*.sh scripts/area-label/tests/*.sh # clean ``` Also green: `check_workflow_pins.py`, `check-org-repo-literals.sh`, `check_agents_md.py`. A `glob_to_regex` bug worth noting, since shellcheck caught it and a test now pins it: a lone backslash written as `'\\'` in a `case` pattern is **two** characters and can never match a one-character input, so a literal `\` in a glob would have gone unescaped into the regex. ## Docs `scripts/area-label/README.md`, `docs/callers/pr-area-label.md` (including the glob semantics and the honest limits — matching is paths-only, and the changed-file list comes from `gh pr view --json files`, which caps at 100), the README catalog row, and the reusable's header contract. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-26 22:38:36 +00:00