Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 11 additions & 3 deletions .github/codeowners/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,17 @@ generated outputs together.
`external_contributors.yaml`. Adding, deleting, or moving unrelated tracked
files cannot rewrite the generated rules.
- Rule precedence follows CODEOWNERS last-match-wins semantics: catch-all and
base area rules, file-type defaults, nested path overrides, then explicit
`shared` rules. Overlapping file-type rules retain their YAML declaration
order, so later entries refine earlier ones.
base area rules, file-type defaults, nested path overrides, explicit
`shared` rules, then `unowned` globs. Overlapping file-type rules retain
their YAML declaration order, so later entries refine earlier ones.
- `unowned:` globs are emitted **last with no owner** — GitHub's ownerless-
pattern semantics — so matching files require no codeowner review at all.
Reserved for deterministic generator outputs whose integrity a CI freshness
gate already guards (hand edits fail `--check`); a human sign-off on such an
artifact adds a reviewer without adding scrutiny, and routes source-only PRs
to unrelated teams. The generators themselves stay owned. The coverage gate
counts unowned globs as claimed, and declaring the same glob `shared` and
`unowned` is rejected.
- Legacy `classify.keyword_rules` are rejected because auto-classification and
keyword co-ownership required a live tree. Declare ownership with explicit
area `path_globs` or `shared` entries.
Expand Down
3 changes: 3 additions & 0 deletions .github/codeowners/areas.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -1098,3 +1098,6 @@ classify:
filetype_rules:
- pattern: '*Dockerfile*'
coowner: ops
unowned:
- docs/fern/pages/reference/api/python/*.mdx
- docs/fern/pages/reference/api/rust/README.mdx
48 changes: 45 additions & 3 deletions .github/codeowners/codeowners_match.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ class ResolvedModel:
shared: list[SharedSpec]
filetype_shared: list[FiletypeShared]
meta: dict = field(default_factory=dict)
unowned: list[str] = field(default_factory=list)

def label_to_team(self) -> dict[str, str]:
return {a.label: a.github_team for a in self.areas}
Expand All @@ -138,6 +139,10 @@ def owned_patterns(self) -> list[str]:
# keep them unanchored in the coverage set too.
for fs in self.filetype_shared:
pats.append(fs.glob)
# Explicitly unowned globs are a deliberate routing decision (review
# exemption), not a coverage hole -- the gate must not report them as
# catch-all-only.
pats.extend(anchor(g) for g in self.unowned)
return pats

def unmatched_paths(self, tree: Iterable[str]) -> list[str]:
Expand Down Expand Up @@ -232,15 +237,20 @@ def resolve_owners(rules: list[tuple[str, list[str]]], filepath: str) -> list[st


def parse_codeowners(text: str) -> list[tuple[str, list[str]]]:
"""Parse a CODEOWNERS file body into ordered ``(pattern, [owner, ...])``."""
"""Parse a CODEOWNERS file body into ordered ``(pattern, [owner, ...])``.

A pattern with no owners is kept as ``(pattern, [])`` -- GitHub treats
such a line as an explicit review exemption (last-match still applies),
so dropping it would make ``resolve_owners`` fall back to whatever owner
an earlier, broader rule declared.
"""
rules: list[tuple[str, list[str]]] = []
for line in text.splitlines():
stripped = line.split("#", 1)[0].strip()
if not stripped:
continue
pattern, *owners = stripped.split()
if owners:
rules.append((pattern, owners))
rules.append((pattern, owners))
return rules


Expand Down Expand Up @@ -313,6 +323,11 @@ def compute_resolution(spec: dict, tree: Iterable[str] | None = None) -> Resolve

* ``areas`` -- ``path_globs`` are emitted verbatim (sorted).
* ``shared`` -- passed through as declared.
* ``unowned`` -- globs emitted LAST with no owner: an explicit
review exemption (GitHub's ownerless-pattern semantics) for content
whose integrity is guarded by CI rather than human review, e.g.
deterministic generator outputs. The coverage gate counts these as
claimed, not as catch-all holes.
* ``advisory`` -- no longer supported. Every owner in CODEOWNERS blocks,
so a leftover ``advisory:`` block, an ``advisory`` key on a ``shared``
entry, or one on a filetype rule, is rejected rather than silently
Expand Down Expand Up @@ -373,6 +388,32 @@ def compute_resolution(spec: dict, tree: Iterable[str] | None = None) -> Resolve
"'advisory' key; shared entries always block -- remove it"
)

raw_unowned = spec.get("unowned", []) or []
if not isinstance(raw_unowned, list):
# A scalar string would iterate per-character below and silently
# emit ownerless rules like ``/d`` ``/o`` ``/c`` ``/s``.
raise SystemExit(
f"areas.yaml: unowned must be a LIST of glob strings, got {raw_unowned!r}"
)
for g in raw_unowned:
Comment on lines +391 to +398

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject scalar unowned policies before iterating

If a future edit uses the natural but invalid scalar spelling unowned: docs/generated/ instead of a YAML list, this loop accepts every character as a valid string and emits ownerless rules such as /d, /o, and /s rather than rejecting the policy. Both regeneration and the drift check consume the same malformed model, while dead globs are only warnings, so CI can pass even though the intended exemption is silently ineffective. Validate that the top-level value is a list before validating its entries.

AGENTS.md reference: AGENTS.md:L140-L143

Useful? React with 👍 / 👎.

if not isinstance(g, str) or not g.strip():
raise SystemExit(
f"areas.yaml: unowned entry {g!r} must be a non-empty glob string"
)
# Store anchored so the conflict check below compares the canonical
# spelling emission uses -- ``docs/x/`` and ``/docs/x/`` are the same
# emitted rule and must collide here (anchor() is idempotent, so the
# emitter and coverage gate re-anchoring is a no-op).
unowned = sorted({anchor(g) for g in raw_unowned})
shared_globs = {anchor(s["glob"]) for s in spec_shared}
conflict = shared_globs & set(unowned)
if conflict:
raise SystemExit(
"areas.yaml: glob(s) declared both shared and unowned "
f"({sorted(conflict)}); pick one -- unowned is emitted last and "
"would silently strip the shared owners"
)
Comment on lines +391 to +415

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Safety check that stops an exemption from silently removing reviewers can be bypassed by writing the path slightly differently

The check that rejects a path listed both as shared and as review-exempt compares the two spellings literally (shared_globs & set(unowned) at .github/codeowners/codeowners_match.py:398-405) before either is normalized, so the same directory written with and without a leading slash slips through and the exemption silently removes the shared reviewers.
Impact: A directory intended to require multi-team review can end up requiring no review at all, with no error reported.

Anchoring happens after the conflict check, so equivalent globs compare unequal

Both sections are anchored at use time: anchor(s["glob"]) for shared (.github/codeowners/emit_codeowners.py:255) and anchor(g) for unowned (.github/codeowners/emit_codeowners.py:360, .github/codeowners/codeowners_match.py:145). So shared: [{glob: "/docs/x/"}] plus unowned: ["docs/x/"] resolve to the identical emitted pattern /docs/x/, yet the conflict set intersection sees two different strings and passes. The unowned line is emitted last, so last-match-wins strips the shared owners — precisely the outcome the guard's error message says it prevents.

Relatedly, the validation at .github/codeowners/codeowners_match.py:392-397 only checks that entries are non-empty after strip() but stores them unstripped, so " docs/x/" is accepted and anchored into / docs/x/, a pattern that matches nothing (reported only as a non-blocking "dead glob" by build_codeowners.py:128).

Suggested change
raw_unowned = spec.get("unowned", []) or []
for g in raw_unowned:
if not isinstance(g, str) or not g.strip():
raise SystemExit(
f"areas.yaml: unowned entry {g!r} must be a non-empty glob string"
)
unowned = sorted(set(raw_unowned))
shared_globs = {s["glob"] for s in spec_shared}
conflict = shared_globs & set(unowned)
if conflict:
raise SystemExit(
"areas.yaml: glob(s) declared both shared and unowned "
f"({sorted(conflict)}); pick one -- unowned is emitted last and "
"would silently strip the shared owners"
)
raw_unowned = spec.get("unowned", []) or []
for g in raw_unowned:
if not isinstance(g, str) or not g.strip():
raise SystemExit(
f"areas.yaml: unowned entry {g!r} must be a non-empty glob string"
)
unowned = sorted({g.strip() for g in raw_unowned})
shared_globs = {anchor(s["glob"].strip()) for s in spec_shared}
conflict = shared_globs & {anchor(g) for g in unowned}
if conflict:
raise SystemExit(
"areas.yaml: glob(s) declared both shared and unowned "
f"({sorted(conflict)}); pick one -- unowned is emitted last and "
"would silently strip the shared owners"
)
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.


areas = [
ResolvedArea(
label=a["label"],
Expand Down Expand Up @@ -404,6 +445,7 @@ def compute_resolution(spec: dict, tree: Iterable[str] | None = None) -> Resolve
shared=list(spec_shared),
filetype_shared=filetype_shared,
meta=dict(spec.get("meta", {})),
unowned=unowned,
)


Expand Down
18 changes: 17 additions & 1 deletion .github/codeowners/emit_codeowners.py
Original file line number Diff line number Diff line change
Expand Up @@ -356,11 +356,25 @@ def fmt(path: str, team: str) -> str:
]
lines += [fmt(p, deco(t)) for p, t in shared_rules]

unowned_rules = sorted(
(anchor(g) for g in model.unowned), key=lambda p: (len(p), p)
)
if unowned_rules:
lines += [
"",
"# --- Explicitly unowned: no codeowner review required (wins via last-match).",
"# Deterministic generator outputs whose integrity is guarded by CI",
"# (freshness --check gates), so human review of the artifact is redundant",
"# and would route source-only PRs to unrelated reviewers. ---",
]
lines += unowned_rules

stats = {
"base": len(base_rules),
"shared": len(shared_rules),
"filetype": len(ft_rules),
"overrides": len(overrides),
"unowned": len(unowned_rules),
"teams": len(teams),
}
return lines, stats
Expand Down Expand Up @@ -413,11 +427,13 @@ def main() -> int:
stats["base"]
+ stats["shared"]
+ stats["filetype"]
+ stats["unowned"]
+ (1 if model.catch_all else 0)
)
print(
f"wrote {args.out} | rules: {total} (base {stats['base']} | "
f"shared {stats['shared']} | file-type {stats['filetype']}) | "
f"shared {stats['shared']} | file-type {stats['filetype']} | "
f"unowned {stats['unowned']}) | "
f"overrides pulled out: {stats['overrides']} | "
f"teams referenced: {stats['teams']}"
)
Expand Down
105 changes: 105 additions & 0 deletions .github/codeowners/test_codeowners.py
Original file line number Diff line number Diff line change
Expand Up @@ -1102,3 +1102,108 @@ def test_no_externals_is_unchanged(self) -> None:
model = self._model()
plain, _ = _render_codeowners(model, group=True, external=[])
assert not any("@jane" in ln for ln in plain)


class TestUnowned:
"""``unowned:`` -- explicit review exemption for CI-guarded generated files."""

def _spec(self) -> dict:
return {
"meta": {"catch_all": "@root"},
"areas": [
{
"label": "docs",
"github_team": "@docs",
"path_globs": ["docs/"],
},
],
"shared": [],
"unowned": ["docs/fern/pages/reference/api/python/"],
}

def _rendered(self, spec: dict) -> str:
model = compute_resolution(spec)
lines, _ = _render_codeowners(model, group=True, external=[])
return "\n".join(lines) + "\n"

def test_unowned_line_wins_last_match_with_no_owner(self) -> None:
rules = parse_codeowners(self._rendered(self._spec()))
assert (
resolve_owners(rules, "docs/fern/pages/reference/api/python/frontend.mdx")
== []
)
# Everything else under docs/ stays docs-owned.
assert resolve_owners(rules, "docs/fern/scripts/gen_python_api.py") == ["@docs"]
assert resolve_owners(rules, "docs/fern/pages/reference/api/README.mdx") == [
"@docs"
]

def test_unowned_is_emitted_after_shared(self) -> None:
spec = self._spec()
spec["shared"] = [
{"glob": "docs/fern/pages/", "owners": ["docs"]},
]
rendered = self._rendered(spec)
shared_idx = rendered.index("/docs/fern/pages/ ")
unowned_idx = rendered.index("/docs/fern/pages/reference/api/python/\n")
assert unowned_idx > shared_idx

def test_unowned_counts_as_claimed_for_coverage(self) -> None:
model = compute_resolution(self._spec())
tree = [
"docs/fern/pages/reference/api/python/frontend.mdx",
"src/orphan.rs",
]
assert model.unmatched_paths(tree) == ["src/orphan.rs"]

def test_parse_codeowners_keeps_ownerless_rules(self) -> None:
rules = parse_codeowners("* @root\n/generated/\n")
assert rules == [("*", ["@root"]), ("/generated/", [])]

def test_glob_both_shared_and_unowned_is_fatal(self) -> None:
spec = self._spec()
spec["shared"] = [
{"glob": "docs/fern/pages/reference/api/python/", "owners": ["docs"]},
]
with pytest.raises(SystemExit, match="shared and unowned"):
compute_resolution(spec)

def test_empty_unowned_entry_is_fatal(self) -> None:
spec = self._spec()
spec["unowned"] = [" "]
with pytest.raises(SystemExit, match="non-empty glob"):
compute_resolution(spec)

def test_scalar_unowned_is_fatal(self) -> None:
# A scalar would iterate per-character and emit /d /o /c /s rules.
spec = self._spec()
spec["unowned"] = "docs/generated/"
with pytest.raises(SystemExit, match="LIST of glob strings"):
compute_resolution(spec)

def test_shared_unowned_conflict_detected_across_anchor_spellings(self) -> None:
# docs/x/ and /docs/x/ are the same emitted rule; the guard must
# compare canonical (anchored) forms, not raw spellings.
spec = self._spec()
spec["unowned"] = ["/docs/fern/pages/reference/api/python/"]
spec["shared"] = [
{"glob": "docs/fern/pages/reference/api/python/", "owners": ["docs"]},
]
with pytest.raises(SystemExit, match="shared and unowned"):
compute_resolution(spec)

def test_star_glob_exempts_only_direct_mdx_files(self) -> None:
spec = self._spec()
spec["unowned"] = ["docs/fern/pages/reference/api/python/*.mdx"]
rules = parse_codeowners(self._rendered(spec))
assert (
resolve_owners(rules, "docs/fern/pages/reference/api/python/frontend.mdx")
== []
)
# A smuggled non-generated file in the same dir stays docs-owned.
assert resolve_owners(
rules, "docs/fern/pages/reference/api/python/custom.txt"
) == ["@docs"]
assert resolve_owners(
rules, "docs/fern/pages/reference/api/python/sub/deep.mdx"
) == ["@docs"]
2 changes: 1 addition & 1 deletion .github/codeowners/who_owns.py
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ def main() -> int:
owners_str = (
" ".join(owners)
if owners
else "(no owner -- falls through; CI coverage gate should block this)"
else "(no owner -- explicitly unowned or uncovered; no codeowner review requested)"
)
print(f"{f}\n review: {owners_str}")

Expand Down
7 changes: 7 additions & 0 deletions CODEOWNERS
Original file line number Diff line number Diff line change
Expand Up @@ -720,3 +720,10 @@
/components/src/dynamo/common/memory/multimodal_embedding_cache_manager.py @ai-dynamo/dynamo-runtime-codeowners @ai-dynamo/dynamo-multimodal-codeowners
/components/src/dynamo/mocker/utils/planner_profiler_perf_data_converter.py @ai-dynamo/dynamo-performance-codeowners @ai-dynamo/dynamo-planner-codeowners
/tests/fault_tolerance/hardware/fault_injection_service/api_service/main.py @ai-dynamo/dynamo-fault-tolerance-codeowners @ai-dynamo/dynamo-observability-codeowners

# --- Explicitly unowned: no codeowner review required (wins via last-match).
# Deterministic generator outputs whose integrity is guarded by CI
# (freshness --check gates), so human review of the artifact is redundant
# and would route source-only PRs to unrelated reviewers. ---
/docs/fern/pages/reference/api/python/*.mdx
/docs/fern/pages/reference/api/rust/README.mdx
Loading