diff --git a/.github/external-labels.json b/.github/external-labels.json new file mode 100644 index 00000000..d079a281 --- /dev/null +++ b/.github/external-labels.json @@ -0,0 +1,6 @@ +[ + { + "name": "bot-reviewed", + "owner": "agent-ci-infra-bot[bot]" + } +] diff --git a/docs/agent-collaboration.md b/docs/agent-collaboration.md index b0f9864c..729d44db 100644 --- a/docs/agent-collaboration.md +++ b/docs/agent-collaboration.md @@ -223,6 +223,23 @@ repository. The nightly `safety-net` job reports drift between the repository la `labels.json`, and any label that an Issue template or `tools/skill_eval_*_config.json` references but `labels.json` lacks. No workflow writes labels. +[`external-labels.json`](../.github/external-labels.json) names the labels that an outside +GitHub App owns and applies, with the app that owns each one. Those names are exempt from the +`group:value` rule and from the drift report, because this repository neither creates them nor +decides their wording: renaming or deleting one only makes the app recreate it. Label +maintenance leaves them alone, and they carry no coordination meaning here, so the rules above +do not apply to them. A name cannot be in both files. An Issue template or +`tools/skill_eval_*_config.json` still may not reference one, since this repository must not +depend on a label it does not own. + +Each `owner` is a required, informational attribution: the reviewer's record of which app +applies that label, so a later reader can tell why the name is exempt rather than missing. No +check verifies it, and none can — GitHub does not report which app created a label, and an +app's configuration is outside this repository. Treat an entry as evidence of a past decision, +not as a verified fact: before relying on one, confirm from the label's own event history that +the named app still applies it. Adding an entry is a scope decision, so it goes through review +like any other change to this file. + ## Evidence refresh — hypothetical example **Hypothetical illustration only; it records no actual PR, run or result in this diff --git a/tests/test_check_labels.py b/tests/test_check_labels.py index 1f633b50..f5605b68 100644 --- a/tests/test_check_labels.py +++ b/tests/test_check_labels.py @@ -7,6 +7,8 @@ ROOT = Path(__file__).resolve().parents[1] MANIFEST = json.loads((ROOT / ".github/labels.json").read_text(encoding="utf-8")) +EXTERNAL = json.loads((ROOT / ".github/external-labels.json").read_text(encoding="utf-8")) +EXTERNAL_NAMES = {entry["name"] for entry in EXTERNAL} LIVE = [{"name": entry["name"], "description": entry["description"]} for entry in MANIFEST] @@ -23,12 +25,16 @@ def test_repository_manifest_is_valid(): assert check.manifest_problems(MANIFEST) == [] +def test_repository_external_list_is_valid(): + assert check.external_problems(EXTERNAL, MANIFEST) == [] + + def test_templates_and_configs_reference_only_listed_labels(): references = check.referenced_labels(ROOT) assert references[".github/ISSUE_TEMPLATE/scoped-task.md"] == ["kind:task"] assert references[".github/ISSUE_TEMPLATE/campaign-acceptance.yml"] == ["kind:acceptance"] assert references["tools/skill_eval_auto_config.json"] == ["skill-eval:execute"] - assert check.drift(MANIFEST, LIVE, references) == [] + assert check.drift(MANIFEST, LIVE, references, EXTERNAL_NAMES) == [] @pytest.mark.parametrize("bad, message", [ @@ -49,11 +55,73 @@ def test_duplicate_names_are_reported(): ] +@pytest.mark.parametrize("bad", ["kind:bug", None, [], 7, True]) +def test_nonobject_entries_are_reported_in_both_manifests(bad): + assert check.manifest_problems([entry("kind:bug"), bad]) == [ + "labels.json entry 2: expected an object" + ] + assert check.external_problems([{"name": "bot-reviewed", "owner": "app"}, bad], []) == [ + "external-labels.json entry 2: expected an object" + ] + + +def test_invalid_entries_do_not_hide_duplicates_or_overlap(): + manifest = [None, entry("kind:bug"), entry("kind:bug")] + assert check.manifest_problems(manifest) == [ + "labels.json entry 1: expected an object", + "kind:bug: listed more than once", + ] + external = [None, {"name": "kind:bug", "owner": "app"}, + {"name": "kind:bug", "owner": "app"}] + assert check.external_problems(external, manifest) == [ + "external-labels.json entry 1: expected an object", + "kind:bug: listed more than once", + "kind:bug: external labels cannot also be in labels.json", + ] + + +@pytest.mark.parametrize("name", [["a", "b"], {"a": 1}, 7, None]) +def test_unusable_managed_names_do_not_reach_duplicate_checks(name): + bad = {"name": name, "group": "kind", "description": "Meaning"} + problems = check.manifest_problems([bad, bad]) + assert len(problems) == 2 + assert all("name must have the form" in problem for problem in problems) + + +@pytest.mark.parametrize("bad, message", [ + ({"name": "bot-reviewed"}, "exactly name and owner"), + ({"name": "bot-reviewed", "owner": "a", "note": "b"}, "exactly name and owner"), + ({"name": "", "owner": "a"}, "nonempty string"), + ({"name": " ", "owner": "a"}, "nonempty string"), + ({"name": None, "owner": "a"}, "nonempty string"), + ({"name": "bot-reviewed", "owner": " "}, "owner must name the app"), +]) +def test_invalid_external_entries_are_reported(bad, message): + [problem] = check.external_problems([bad], []) + assert message in problem + + +@pytest.mark.parametrize("name", [["a", "b"], {"a": 1}, 7]) +def test_an_unhashable_or_nonstring_external_name_is_reported_not_raised(name): + # An unusable name must not reach the duplicate/overlap sets and crash the check. + [problem] = check.external_problems([{"name": name, "owner": "app"}], [entry("kind:bug")]) + assert "name must be a nonempty string" in problem + + +def test_external_duplicates_and_overlap_with_the_manifest_are_reported(): + external = [{"name": "bot-reviewed", "owner": "app"}, {"name": "bot-reviewed", "owner": "app"}, + {"name": "kind:bug", "owner": "app"}] + assert check.external_problems(external, [entry("kind:bug")]) == [ + "bot-reviewed: listed more than once", + "kind:bug: external labels cannot also be in labels.json", + ] + + def test_drift_reports_missing_extra_changed_and_unlisted_references(): manifest = [entry("kind:bug"), entry("status:proposed", description="Waiting")] repository = [{"name": "status:proposed", "description": None}, {"name": "bug", "description": "Something isn't working"}] - assert check.drift(manifest, repository, {"form.yml": ["kind:bug", "acceptance"]}) == [ + assert check.drift(manifest, repository, {"form.yml": ["kind:bug", "acceptance"]}, set()) == [ "kind:bug: in labels.json, missing from the repository", "bug: in the repository, missing from labels.json", "status:proposed: repository description differs from labels.json", @@ -61,6 +129,25 @@ def test_drift_reports_missing_extra_changed_and_unlisted_references(): ] +def test_drift_accepts_an_external_label_in_the_repository(): + manifest = [entry("kind:bug")] + repository = [{"name": "kind:bug", "description": "Meaning"}, + {"name": "bot-reviewed", "description": None}] + assert check.drift(manifest, repository, {}, {"bot-reviewed"}) == [] + # Absence is also accepted: an outside app owns whether the label exists. + assert check.drift(manifest, repository[:1], {}, {"bot-reviewed"}) == [] + + +def test_drift_still_rejects_a_template_reference_to_an_external_label(): + manifest = [entry("kind:bug")] + repository = [{"name": "kind:bug", "description": "Meaning"}, + {"name": "bot-reviewed", "description": None}] + assert check.drift(manifest, repository, {"form.yml": ["bot-reviewed"]}, + {"bot-reviewed"}) == [ + "bot-reviewed: referenced by form.yml, missing from labels.json", + ] + + def test_template_and_config_label_forms(tmp_path): templates = tmp_path / ".github/ISSUE_TEMPLATE" templates.mkdir(parents=True) @@ -89,7 +176,8 @@ def test_markdown_template_without_front_matter_fails(tmp_path): def test_main_passes_silently_when_repository_matches(tmp_path, capsys): - assert check.main([write_lines(tmp_path / "labels.jsonl", LIVE)]) == 0 + live = LIVE + [{"name": name, "description": None} for name in sorted(EXTERNAL_NAMES)] + assert check.main([write_lines(tmp_path / "labels.jsonl", live)]) == 0 assert capsys.readouterr().out == "" @@ -102,3 +190,21 @@ def test_main_reports_each_drift_line_and_fails(tmp_path, capsys): "label drift: ::error::x: in the repository, missing from labels.json", ] assert "labels.json" in out[2] and len(out) == 3 + + +@pytest.mark.parametrize("filename", ["labels.json", "external-labels.json"]) +@pytest.mark.parametrize("bad", ["kind:bug", None]) +def test_main_reports_nonobject_entries_and_fails_without_drift( + tmp_path, monkeypatch, capsys, filename, bad): + github = tmp_path / ".github" + github.mkdir() + for name, entries in [("labels.json", MANIFEST), ("external-labels.json", EXTERNAL)]: + (github / name).write_text(json.dumps([bad] if name == filename else entries), + encoding="utf-8") + monkeypatch.setattr(check, "ROOT", tmp_path) + # No ISSUE_TEMPLATE directory exists: drift/reference reads must not run after validation fails. + assert check.main([write_lines(tmp_path / "labels.jsonl", LIVE)]) == 1 + assert capsys.readouterr().out.splitlines() == [ + f"label drift: {filename} entry 1: expected an object", + check.ADVICE, + ] diff --git a/tools/check_labels.py b/tools/check_labels.py index 5f118a36..c2dd10ec 100644 --- a/tools/check_labels.py +++ b/tools/check_labels.py @@ -21,7 +21,10 @@ def manifest_problems(manifest: list[dict]) -> list[str]: problems = [] - for entry in manifest: + for index, entry in enumerate(manifest, start=1): + if not isinstance(entry, dict): + problems.append(f"labels.json entry {index}: expected an object") + continue name = entry.get("name") if set(entry) != {"name", "group", "description"}: problems.append(f"{name}: expected exactly name, group and description") @@ -32,12 +35,40 @@ def manifest_problems(manifest: list[dict]) -> list[str]: description = entry["description"] if not isinstance(description, str) or not 0 < len(description) <= MAX_DESCRIPTION: problems.append(f"{name}: description must have 1-{MAX_DESCRIPTION} characters") - names = [entry.get("name") for entry in manifest] + names = [entry.get("name") for entry in manifest + if isinstance(entry, dict) and isinstance(entry.get("name"), str)] problems += [f"{name}: listed more than once" for name in sorted({name for name in names if names.count(name) > 1}, key=str)] return problems +def external_problems(external: list[dict], manifest: list[dict]) -> list[str]: + problems = [] + names = [] + for index, entry in enumerate(external, start=1): + if not isinstance(entry, dict): + problems.append(f"external-labels.json entry {index}: expected an object") + continue + name = entry.get("name") + if set(entry) != {"name", "owner"}: + problems.append(f"{name}: expected exactly name and owner") + continue + # Only a usable name joins the duplicate and overlap comparisons below. + if isinstance(name, str) and name.strip(): + names.append(name) + else: + problems.append(f"{name}: name must be a nonempty string") + if not isinstance(entry["owner"], str) or not entry["owner"].strip(): + problems.append(f"{name}: owner must name the app that applies it") + problems += [f"{name}: listed more than once" + for name in sorted({n for n in names if names.count(n) > 1})] + listed = {e["name"] for e in manifest + if isinstance(e, dict) and isinstance(e.get("name"), str)} + problems += [f"{name}: external labels cannot also be in labels.json" + for name in sorted(set(names) & listed)] + return problems + + def _names(value, source: str) -> list[str]: # GitHub accepts a list of names or one comma-separated string in templates. if value is None: @@ -72,14 +103,14 @@ def referenced_labels(root: Path) -> dict[str, list[str]]: def drift(manifest: list[dict], repository: list[dict], - references: dict[str, list[str]]) -> list[str]: + references: dict[str, list[str]], external: set[str]) -> list[str]: expected = {entry["name"]: entry["description"] for entry in manifest} # The labels API returns null for a label without a description. actual = {label["name"]: label["description"] or "" for label in repository} problems = [f"{name}: in labels.json, missing from the repository" for name in sorted(expected.keys() - actual.keys())] problems += [f"{name}: in the repository, missing from labels.json" - for name in sorted(actual.keys() - expected.keys())] + for name in sorted(actual.keys() - expected.keys() - external)] problems += [f"{name}: repository description differs from labels.json" for name in sorted(expected.keys() & actual.keys()) if expected[name] != actual[name]] @@ -95,11 +126,13 @@ def main(argv: list[str] | None = None) -> int: help="JSON lines holding each repository label's name and description") args = parser.parse_args(argv) manifest = json.loads((ROOT / ".github/labels.json").read_text(encoding="utf-8")) + external = json.loads((ROOT / ".github/external-labels.json").read_text(encoding="utf-8")) repository = [json.loads(line) for line in args.repository_labels.read_text(encoding="utf-8").splitlines() if line.strip()] - problems = manifest_problems(manifest) + problems = manifest_problems(manifest) + external_problems(external, manifest) if not problems: - problems = drift(manifest, repository, referenced_labels(ROOT)) + problems = drift(manifest, repository, referenced_labels(ROOT), + {entry["name"] for entry in external}) # Each printed line starts with "label drift" so a label name cannot form a workflow command. for problem in problems: print(f"label drift: {problem}")