From c8631deda914e5c6a0c0ac9ba1fd9ffb956307c1 Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 23 Apr 2026 15:52:14 -0700 Subject: [PATCH 1/2] fix: map author_association NONE to unapproved integrity GitHub's author_association NONE means 'no association with the repo' which does NOT imply the user is established or trustworthy. Previously NONE mapped to the lowest integrity level (none), same as FIRST_TIMER. Now NONE maps to unapproved (reader_integrity), matching FIRST_TIME_CONTRIBUTOR and CONTRIBUTOR, since all three represent users who are not brand-new to GitHub but have no special repo relationship. Only FIRST_TIMER ('has not previously committed to GitHub') and unknown/missing values remain at the none baseline. Also fixes pre-existing TestRegisterFlagCompletions failures by checking GetFlagCompletionFunc instead of flag.Annotations (RegisterFlagCompletionFunc does not populate annotations). Closes #4419 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- .../github-guard/docs/INTEGRITY_TAG_SPEC.md | 32 ++++++-- guards/github-guard/docs/TESTING.md | 12 ++- .../rust-guard/src/labels/helpers.rs | 74 ++++++++++++++++++- .../github-guard/rust-guard/src/labels/mod.rs | 41 +++++----- internal/cmd/flags_test.go | 38 +++++----- 5 files changed, 150 insertions(+), 47 deletions(-) diff --git a/guards/github-guard/docs/INTEGRITY_TAG_SPEC.md b/guards/github-guard/docs/INTEGRITY_TAG_SPEC.md index 5bd5c6f3..9ea4304f 100644 --- a/guards/github-guard/docs/INTEGRITY_TAG_SPEC.md +++ b/guards/github-guard/docs/INTEGRITY_TAG_SPEC.md @@ -54,12 +54,34 @@ Integrity assignment is derived from a combination of: ### Author Association Initialization `author_association` provides the **initial integrity floor** for user-authored objects. +Values are defined by the GitHub API +([reference](https://docs.github.com/en/graphql/reference/enums#commentauthorassociation)). Initialization mapping: - `OWNER`, `MEMBER`, `COLLABORATOR` -> `approved:` -- `CONTRIBUTOR`, `FIRST_TIME_CONTRIBUTOR` -> `unapproved:` -- `FIRST_TIMER`, `NONE` -> `[]` (initial floor before baseline enforcement) +- `CONTRIBUTOR`, `FIRST_TIME_CONTRIBUTOR`, `NONE` -> `unapproved:` +- `FIRST_TIMER`, missing, unknown -> `[]` (initial floor before baseline enforcement) + +**Rationale for `NONE` → `unapproved`:** + +GitHub's definitions for `FIRST_TIMER`, `FIRST_TIME_CONTRIBUTOR`, and `NONE` are +intentionally vague. Per the +[CommentAuthorAssociation docs](https://docs.github.com/en/graphql/reference/enums#commentauthorassociation): + +| Value | GitHub definition | +|---|---| +| `FIRST_TIMER` | "Author has not previously committed to **GitHub**." | +| `FIRST_TIME_CONTRIBUTOR` | "Author has not previously committed to **the repository**." | +| `NONE` | "Author has **no association** with the repository." | + +`NONE` does **not** mean the user is established or trustworthy — only that they have +no special relationship with the repository. However, `NONE` is distinct from +`FIRST_TIMER` (brand-new to all of GitHub). In practice `NONE` covers users who have +opened issues or commented but never committed, as well as users active elsewhere on +GitHub. We treat `NONE` the same as `FIRST_TIME_CONTRIBUTOR` because both represent +users with no prior contributions to the specific repo who are not brand-new to GitHub. +Only `FIRST_TIMER` indicates a truly new GitHub account. Notes: @@ -84,7 +106,7 @@ The label cannot be downgraded below its initialized level. Example: an issue au ### Public repositories -- Issues (and issue-like user-submitted discussion objects) initialize from `author_association` and are commonly baseline `none:` for `NONE`/`FIRST_TIMER` after baseline enforcement. +- Issues (and issue-like user-submitted discussion objects) initialize from `author_association` and are commonly baseline `unapproved:` for `NONE` or `none:` for `FIRST_TIMER` after baseline enforcement. - Pull requests: - Forked PR (head repo differs from base repo): endorsement baseline `unapproved:` - Direct PR (head repo == base repo): endorsement baseline `approved:` @@ -99,7 +121,7 @@ Resource labels are coarse pre-check labels by tool call. | Tool / Resource Type | Private Repo | Public Repo | |---|---|---| -| `get_issue`, `list_issues` | max(author_association floor, approved) | author_association floor (common NONE/FIRST_TIMER => baseline `none`) | +| `get_issue`, `list_issues` | max(author_association floor, approved) | author_association floor (NONE => baseline `unapproved`, FIRST_TIMER => baseline `none`) | | `search_issues` | baseline `none` (cross-repo coarse) | baseline `none` | | `get_pull_request`, `list_pull_requests` | max(author_association floor, approved); merged/default-branch evidence can elevate to merged | start from author_association floor, then apply PR lineage baseline (direct => approved, forked => unapproved); merged/default-branch evidence can elevate to merged | | `search_pull_requests` | baseline `none` (cross-repo coarse) | baseline `none` | @@ -131,7 +153,7 @@ Response labels are fine-grained per item and are authoritative when available. | Response Object Type | Private Repo | Public Repo | |---|---|---| | Repository item (`search_repositories`, `get_repository`) | approved | approved | -| Issue item (`list_issues`, `search_issues`, `get_issue`) | max(author_association floor, approved) | author_association floor (commonly baseline `none` for `NONE`/`FIRST_TIMER`) | +| Issue item (`list_issues`, `search_issues`, `get_issue`) | max(author_association floor, approved) | author_association floor (NONE => `unapproved`, FIRST_TIMER => `none`) | | Pull request item (`list_pull_requests`, `search_pull_requests`, `get_pull_request`) | max(author_association floor, approved); if merged/default-branch reachable => merged | start from author_association floor; apply lineage baseline (direct => approved, forked => unapproved); if merged/default-branch reachable => merged | | Commit item (`list_commits`, `get_commit`) | max(author_association floor, approved); if default-branch reachable => merged | author_association floor; if default-branch reachable => merged; otherwise stay at floor unless other endorsement evidence applies | | File content item (`get_file_contents`) | default/no-ref: merged; otherwise approved | default/no-ref: merged; otherwise approved | diff --git a/guards/github-guard/docs/TESTING.md b/guards/github-guard/docs/TESTING.md index a3b9140e..d9f1d085 100644 --- a/guards/github-guard/docs/TESTING.md +++ b/guards/github-guard/docs/TESTING.md @@ -310,10 +310,20 @@ integrity floor (case-insensitive) as follows: - `COLLABORATOR` → approved - `CONTRIBUTOR` → unapproved - `FIRST_TIME_CONTRIBUTOR` → unapproved +- `NONE` → unapproved - `FIRST_TIMER` → none -- `NONE` → none - missing/unknown value → none +**Note on `NONE` vs `FIRST_TIMER`:** GitHub's API definitions for these values are +intentionally vague +([reference](https://docs.github.com/en/graphql/reference/enums#commentauthorassociation)). +`FIRST_TIMER` ("Author has not previously committed to GitHub") indicates a brand-new +GitHub account. `NONE` ("Author has no association with the repository") does **not** +imply the user is established or trustworthy — only that they are not `FIRST_TIMER`. +We map `NONE` to `unapproved` (same as `FIRST_TIME_CONTRIBUTOR`) because both +represent users with no prior contributions to the specific repo who are not brand-new +to GitHub. + In the implementation: - approved floor uses `writer_integrity(scope)` - unapproved floor uses `reader_integrity(scope)` diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index e93b4728..bfb975fb 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -1225,8 +1225,30 @@ pub fn max_integrity( /// /// Mapping (case-insensitive): /// - OWNER, MEMBER, COLLABORATOR => approved -/// - CONTRIBUTOR, FIRST_TIME_CONTRIBUTOR => unapproved -/// - FIRST_TIMER, NONE, missing => none +/// - CONTRIBUTOR, FIRST_TIME_CONTRIBUTOR, NONE => unapproved +/// - FIRST_TIMER, missing, unknown => none +/// +/// ### `NONE` vs `FIRST_TIMER` +/// +/// GitHub's API definitions for these values are intentionally vague +/// (see ): +/// +/// - `FIRST_TIMER`: "Author has not previously committed to GitHub." +/// This indicates a brand-new GitHub account with no commit history anywhere. +/// +/// - `FIRST_TIME_CONTRIBUTOR`: "Author has not previously committed to the repository." +/// The user has committed elsewhere on GitHub but not to this specific repo. +/// +/// - `NONE`: "Author has no association with the repository." +/// This does **not** mean the user is established or trustworthy — only that +/// they have no special relationship with the repo. In practice `NONE` covers +/// users who have opened issues or commented but never committed, as well as +/// accounts that have simply never interacted before. +/// +/// We map `NONE` to `unapproved` (same as `FIRST_TIME_CONTRIBUTOR`) because +/// both represent users with no prior contributions to the specific repo who +/// are not brand-new to GitHub. The only value that indicates a truly new +/// GitHub account is `FIRST_TIMER`. pub fn author_association_floor_from_str( scope: &str, association: Option<&str>, @@ -1239,8 +1261,8 @@ pub fn author_association_floor_from_str( let normalized = raw.trim().to_ascii_uppercase(); match normalized.as_str() { "OWNER" | "MEMBER" | "COLLABORATOR" => writer_integrity(scope, ctx), - "CONTRIBUTOR" | "FIRST_TIME_CONTRIBUTOR" => reader_integrity(scope, ctx), - _ => vec![], // FIRST_TIMER, NONE, or any unrecognised value + "CONTRIBUTOR" | "FIRST_TIME_CONTRIBUTOR" | "NONE" => reader_integrity(scope, ctx), + _ => vec![], // FIRST_TIMER or any unrecognised value } } @@ -2184,6 +2206,50 @@ mod tests { assert_eq!(integrity_for_level("unknown", scope, &ctx), none_integrity(scope, &ctx)); } + #[test] + fn test_author_association_none_maps_to_unapproved() { + // NONE means "no association with the repository" — NOT "brand new to GitHub". + // It should map to unapproved (reader_integrity), same as FIRST_TIME_CONTRIBUTOR. + // See https://docs.github.com/en/graphql/reference/enums#commentauthorassociation + let ctx = test_ctx(); + let scope = "owner/repo"; + + // NONE → unapproved (reader_integrity) + assert_eq!( + author_association_floor_from_str(scope, Some("NONE"), &ctx), + reader_integrity(scope, &ctx), + "NONE should map to unapproved (reader) integrity" + ); + + // FIRST_TIME_CONTRIBUTOR → unapproved (same as NONE) + assert_eq!( + author_association_floor_from_str(scope, Some("FIRST_TIME_CONTRIBUTOR"), &ctx), + reader_integrity(scope, &ctx), + "FIRST_TIME_CONTRIBUTOR should map to unapproved (reader) integrity" + ); + + // FIRST_TIMER → none (brand-new GitHub account) + assert_eq!( + author_association_floor_from_str(scope, Some("FIRST_TIMER"), &ctx), + vec![] as Vec, + "FIRST_TIMER should map to none (empty) integrity" + ); + + // NONE and FIRST_TIME_CONTRIBUTOR produce the same integrity + assert_eq!( + author_association_floor_from_str(scope, Some("NONE"), &ctx), + author_association_floor_from_str(scope, Some("FIRST_TIME_CONTRIBUTOR"), &ctx), + "NONE and FIRST_TIME_CONTRIBUTOR should produce identical integrity" + ); + + // NONE and FIRST_TIMER produce different integrity + assert_ne!( + author_association_floor_from_str(scope, Some("NONE"), &ctx), + author_association_floor_from_str(scope, Some("FIRST_TIMER"), &ctx), + "NONE and FIRST_TIMER should produce different integrity levels" + ); + } + #[test] fn test_effective_disapproval_integrity_defaults_to_none() { let ctx = PolicyContext::default(); diff --git a/guards/github-guard/rust-guard/src/labels/mod.rs b/guards/github-guard/rust-guard/src/labels/mod.rs index 20fc36a2..58d291b4 100644 --- a/guards/github-guard/rust-guard/src/labels/mod.rs +++ b/guards/github-guard/rust-guard/src/labels/mod.rs @@ -818,7 +818,7 @@ mod tests { let unknown_commit = json!({"author_association": "NONE"}); assert_eq!( commit_integrity(&unknown_commit, repo, false, false, &ctx), - none_integrity(repo, &ctx) + reader_integrity(repo, &ctx) ); assert_eq!( commit_integrity(&unknown_commit, repo, true, false, &ctx), @@ -925,14 +925,15 @@ mod tests { writer_integrity(repo, &ctx) ); - // Non-trusted bot still gets none integrity on public repo + // Non-trusted bot gets unapproved (reader) integrity on public repo + // because NONE association maps to unapproved let renovate_issue = json!({ "user": {"login": "renovate[bot]"}, "author_association": "NONE" }); assert_eq!( issue_integrity(&renovate_issue, repo, false, &ctx), - none_integrity(repo, &ctx) + reader_integrity(repo, &ctx) ); } @@ -1064,11 +1065,12 @@ mod tests { writer_integrity(repo, &ctx_with_bots) ); - // Without trusted_bots in context, the same bot gets none integrity + // Without trusted_bots in context, the same bot gets unapproved (reader) + // integrity because NONE association maps to unapproved let ctx_without_bots = default_ctx(); assert_eq!( issue_integrity(&configured_bot_issue, repo, false, &ctx_without_bots), - none_integrity(repo, &ctx_without_bots) + reader_integrity(repo, &ctx_without_bots) ); } @@ -1091,11 +1093,11 @@ mod tests { writer_integrity(repo, &ctx_with_bots) ); - // Without trusted_bots, the same bot gets none integrity + // Without trusted_bots, the same bot gets unapproved (reader) integrity let ctx_without_bots = default_ctx(); assert_eq!( pr_integrity(&configured_bot_pr, repo, false, None, &ctx_without_bots), - none_integrity(repo, &ctx_without_bots) + reader_integrity(repo, &ctx_without_bots) ); } @@ -1128,14 +1130,14 @@ mod tests { writer_integrity(repo, &ctx_with_bots) ); - // Unknown bot still gets none integrity + // Unknown bot gets unapproved (reader) integrity because NONE maps to unapproved let unknown_bot_issue = json!({ "user": {"login": "unknown-bot[bot]"}, "author_association": "NONE" }); assert_eq!( issue_integrity(&unknown_bot_issue, repo, false, &ctx_with_bots), - none_integrity(repo, &ctx_with_bots) + reader_integrity(repo, &ctx_with_bots) ); } @@ -4044,11 +4046,12 @@ mod tests { writer_integrity(repo, &ctx_with_users) ); - // Without trusted_users in context, the same user gets none integrity + // Without trusted_users in context, the same user gets unapproved (reader) + // integrity because NONE association maps to unapproved let ctx_without_users = default_ctx(); assert_eq!( issue_integrity(&trusted_user_issue, repo, false, &ctx_without_users), - none_integrity(repo, &ctx_without_users) + reader_integrity(repo, &ctx_without_users) ); } @@ -4071,11 +4074,11 @@ mod tests { writer_integrity(repo, &ctx_with_users) ); - // Without trusted_users, the same user gets none integrity + // Without trusted_users, the same user gets unapproved (reader) integrity let ctx_without_users = default_ctx(); assert_eq!( pr_integrity(&trusted_user_pr, repo, false, None, &ctx_without_users), - none_integrity(repo, &ctx_without_users) + reader_integrity(repo, &ctx_without_users) ); } @@ -5082,10 +5085,10 @@ mod tests { "author_association": "NONE", "number": 10 }); - // No reactions → base integrity (none for external contributor on public repo) + // No reactions → base integrity (unapproved for NONE association on public repo) assert_eq!( issue_integrity(&issue, repo, false, &ctx), - helpers::none_integrity(repo, &ctx) + helpers::reader_integrity(repo, &ctx) ); } @@ -5101,10 +5104,10 @@ mod tests { "number": 11, "reactions": {"total_count": 5, "thumbs_up": 5, "+1": 5} }); - // Gateway mode → fall through to base integrity (none) + // Gateway mode → fall through to base integrity (unapproved for NONE) assert_eq!( issue_integrity(&issue, repo, false, &ctx), - helpers::none_integrity(repo, &ctx) + helpers::reader_integrity(repo, &ctx) ); } @@ -5142,10 +5145,10 @@ mod tests { "number": 30, "reactions": {"nodes": []} }); - // Empty nodes → no change from base integrity + // Empty nodes → no change from base integrity (unapproved for NONE) assert_eq!( issue_integrity(&issue, repo, false, &ctx), - helpers::none_integrity(repo, &ctx) + helpers::reader_integrity(repo, &ctx) ); } diff --git a/internal/cmd/flags_test.go b/internal/cmd/flags_test.go index 201b2dd8..eb52326b 100644 --- a/internal/cmd/flags_test.go +++ b/internal/cmd/flags_test.go @@ -103,41 +103,43 @@ func TestRegisterFlagCompletions(t *testing.T) { t.Run("config flag completion returns toml extension filter", func(t *testing.T) { cmd := setupCmd(t) - flag := cmd.Flags().Lookup("config") - require.NotNil(t, flag, "config flag should be registered") - exts, ok := flag.Annotations[cobra.BashCompFilenameExt] - require.True(t, ok, "config flag should have filename extension annotation") - assert.Equal(t, []string{"toml"}, exts, + compFunc, ok := cmd.GetFlagCompletionFunc("config") + require.True(t, ok, "config flag should have a completion function") + completions, directive := compFunc(cmd, nil, "") + assert.Equal(t, []string{"toml"}, completions, "config flag should complete with .toml extension") + assert.Equal(t, cobra.ShellCompDirectiveFilterFileExt, directive) }) t.Run("log-dir flag completion returns directory filter", func(t *testing.T) { cmd := setupCmd(t) - flag := cmd.Flags().Lookup("log-dir") - require.NotNil(t, flag, "log-dir flag should be registered") - _, ok := flag.Annotations[cobra.BashCompSubdirsInDir] - assert.True(t, ok, "log-dir flag should have directory annotation") + compFunc, ok := cmd.GetFlagCompletionFunc("log-dir") + require.True(t, ok, "log-dir flag should have a completion function") + _, directive := compFunc(cmd, nil, "") + assert.Equal(t, cobra.ShellCompDirectiveFilterDirs, directive, + "log-dir flag should have directory completion directive") }) t.Run("payload-dir flag completion returns directory filter", func(t *testing.T) { cmd := setupCmd(t) - flag := cmd.Flags().Lookup("payload-dir") - require.NotNil(t, flag, "payload-dir flag should be registered") - _, ok := flag.Annotations[cobra.BashCompSubdirsInDir] - assert.True(t, ok, "payload-dir flag should have directory annotation") + compFunc, ok := cmd.GetFlagCompletionFunc("payload-dir") + require.True(t, ok, "payload-dir flag should have a completion function") + _, directive := compFunc(cmd, nil, "") + assert.Equal(t, cobra.ShellCompDirectiveFilterDirs, directive, + "payload-dir flag should have directory completion directive") }) t.Run("env flag completion returns .env extension filter", func(t *testing.T) { cmd := setupCmd(t) - flag := cmd.Flags().Lookup("env") - require.NotNil(t, flag, "env flag should be registered") - exts, ok := flag.Annotations[cobra.BashCompFilenameExt] - require.True(t, ok, "env flag should have filename extension annotation") - assert.Equal(t, []string{"env"}, exts, + compFunc, ok := cmd.GetFlagCompletionFunc("env") + require.True(t, ok, "env flag should have a completion function") + completions, directive := compFunc(cmd, nil, "") + assert.Equal(t, []string{"env"}, completions, "env flag should complete with .env extension") + assert.Equal(t, cobra.ShellCompDirectiveFilterFileExt, directive) }) t.Run("guards-mode flag completion returns valid enum values", func(t *testing.T) { From c72f4c1e8e51415676141f2f26546bb95a9fbc7d Mon Sep 17 00:00:00 2001 From: Landon Cox Date: Thu, 23 Apr 2026 15:56:43 -0700 Subject: [PATCH 2/2] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- guards/github-guard/rust-guard/src/labels/helpers.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/guards/github-guard/rust-guard/src/labels/helpers.rs b/guards/github-guard/rust-guard/src/labels/helpers.rs index bfb975fb..05377557 100644 --- a/guards/github-guard/rust-guard/src/labels/helpers.rs +++ b/guards/github-guard/rust-guard/src/labels/helpers.rs @@ -1226,7 +1226,7 @@ pub fn max_integrity( /// Mapping (case-insensitive): /// - OWNER, MEMBER, COLLABORATOR => approved /// - CONTRIBUTOR, FIRST_TIME_CONTRIBUTOR, NONE => unapproved -/// - FIRST_TIMER, missing, unknown => none +/// - FIRST_TIMER, missing, unknown => [] (the `none:` floor is applied later by baseline enforcement) /// /// ### `NONE` vs `FIRST_TIMER` ///