feat: Prow-style directory-scoped OWNERS files - #138
Merged
Conversation
Adds src/utils/owners.ts: parseOwners (approvers/reviewers lists, options.no_parent_owners, emeritus and unknown keys tolerated, filters noted and ignored, logins lowercased), ownersDir, effectiveOwners (walk from the file's directory to the root taking the union, stopping at no_parent_owners) and loadOwnersTree (recursive git tree at a ref, fetching only the OWNERS blobs in ancestor directories of the paths of interest; a truncated tree falls back to probing each candidate path with the contents API). Not wired into authorization yet, so dist/ is unchanged. Signed-off-by: Jeffrey Sica <me@jeefy.dev>
… the PR base On a pull request, assertAuthorizedByOwnersOrMembership now resolves the OWNERS files covering each changed file (Prow inheritance, including options.no_parent_owners and both sides of a rename) from the PR's base commit, so a PR cannot grant itself approvers: - /approve: the commenter must be an approver for every changed file; the denial names the first uncovered file and the OWNERS consulted. - /lgtm: the commenter must be a reviewer or approver for at least one changed file. - A changed file with no covering OWNERS is an error naming the file. - The org-member/collaborator fallback applies only when the base tree has no OWNERS file at all. Issues keep using the root OWNERS of the default branch via the contents API with unchanged messages; the tree/blob machinery is PR-only. Logins are compared case-insensitively and a role that is not a list is rejected on both paths. Closes #65 Signed-off-by: Jeffrey Sica <me@jeefy.dev>
Rewrite the OWNERS section of docs/commands.md for multiple OWNERS files: resolution walk and no_parent_owners, base-branch reading and why, /approve (every changed file) versus /lgtm (at least one), the issue case, fail-closed rules, when the membership fallback applies, and the supported, ignored and unsupported keys. Point the /lgtm and /approve policy cells at the section and extend the example in docs/examples.md with a nested sdk/OWNERS. Signed-off-by: Jeffrey Sica <me@jeefy.dev>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Closes #65. Implements the design agreed on the issue: multiple
OWNERSfiles across directories with Prow's resolution semantics, read from the PR's base branch, with single-actor authorization ("Model A").Resolution
For a changed file, walk from its directory up to the root collecting every
OWNERS(child ∪ parents);options: { no_parent_owners: true }stops the walk. A root-onlyOWNERScovers everything — today's behaviour is the special case, and issues (non-PR) are unchanged byte-for-byte: same singleGET /contents/OWNERS, same error strings.Authorization on PRs
/approve: commenter must be an approver for every changed file (renames count both old and new paths). Denial names the first uncovered file and the OWNERS consulted./lgtm: commenter must be a reviewer or approver for at least one changed file (Prow's rule).Security
OWNERS are loaded from
pull.base.sha, never the head — a PR cannot add itself toapprovers. Pinned by a test whose PR editsOWNERS.Fail closed
A changed file with no covering OWNERS →
no OWNERS file covers <file>. Any fetch/parse error → error. The org-member/collaborator fallback applies only when the repository has no OWNERS files at all. A role that isn't a list of strings now errors — the old code did"alice".includes(username), a substring match (latent bug, fixed by sharing one parser).Loading
One
git.getTree(base, recursive)+ onegit.getBlobper OWNERS on the changed files' ancestor chains (pruned; blobs for unrelated dirs are not fetched). Typical nested PR: 5 API calls. Truncated tree (>100k entries) → probedir/OWNERSper ancestor directory instead. Logins compare case-insensitively.emeritus_*ignored; unknown keys tolerated;filters,labels, aliases, teams unsupported (documented).Testing
owners.tsat 100%; coverage 91.3 / 92.1 br / 98.2 fn. No existing assertion changed.__tests__/utils/ownersAuth.test.ts(19): root parity · nested inheritance ·no_parent_owners· approve strictness across two dirs · lgtm any-file · uncovered file · zero OWNERS → membership · head OWNERS ignored · issue path untouched · truncated-tree probe · malformed role · renames · pagination · tree/blob errors · ancestor pruning.no_parent_ownersand head-ignored (red output in the delegated report).dist/index.js: nested approver → review created; cross-dir denial → reply comment names the file, no review, exit 1.dist/byte-identical across repeated packs.Out of scope (per #65): aggregated multi-approver approval +
approvedlabel,OWNERS_ALIASES,filters,owners-filepath/URL input.