Give GitHubReposScope a concrete type - #55793
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review.
|
There was a problem hiding this comment.
Pull request overview
Introduces a concrete repository-scope type for GitHub guard policies and simplifies downstream handling.
Changes:
- Normalizes scalar and list scopes into
GitHubReposScope. - Simplifies validation, hashing, and reporting.
- Updates affected tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/tools_types.go |
Defines typed scope and YAML decoding. |
pkg/workflow/tools_parser.go |
Normalizes parsed scope values. |
pkg/workflow/tools_validation_github.go |
Validates typed scopes. |
pkg/workflow/tools_validation_test.go |
Updates validation coverage. |
pkg/workflow/tools_validation_github_test.go |
Updates alias-validation tests. |
pkg/workflow/tools_types_test.go |
Tests normalization and YAML decoding. |
pkg/workflow/cache_integrity.go |
Simplifies scope canonicalization. |
pkg/workflow/cache_integrity_test.go |
Updates policy-hash fixtures. |
pkg/cli/compile_guard_policy_report.go |
Simplifies scope reporting. |
pkg/cli/compile_guard_policy_report_test.go |
Updates report fixtures. |
Review details
- Files reviewed: 10/10 changed files
- Comments generated: 1
- Review effort level: Balanced
| value, ok := repo.(string) | ||
| if !ok { | ||
| return GitHubReposScope{} | ||
| } |
There was a problem hiding this comment.
Addressed by ad17985 and covered again in c60b4f2: parseGitHubReposScope now returns contextual errors through both guard-policy validation and ParseToolsConfig for mixed non-string arrays.
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment.
|
There was a problem hiding this comment.
Request changes
The new concrete GitHubReposScope type is a good cleanup direction, but this patch currently turns malformed allowed-repos input into the implicit default scope instead of rejecting it.
Blocking theme
- Invalid
allowed-reposshapes are normalized to an empty slice during parsing, and later treated as “field omitted”. - That means bad guard-policy configuration can now bypass validation and widen to
allowed-repos: all, which is the opposite of fail-closed behavior for an access-control setting. - Please preserve explicit validation failures for unsupported scalar types and non-string array entries, then add regression coverage for parser + validator integration.
🔎 Code quality review by PR Code Quality Reviewer · pi · gpt54 · 10.1 AIC · ⌖ 6.98 AIC · ⊞ 7K
Comment /review to run again
| for _, repo := range repos { | ||
| value, ok := repo.(string) | ||
| if !ok { | ||
| return GitHubReposScope{} |
There was a problem hiding this comment.
This change silently reclassifies malformed allowed-repos values as “not configured”, which means invalid frontmatter can now bypass the min-integrity requirement and get defaulted to all instead of failing fast.
💡 Why this blocks merge
Before this patch, an unsupported type like allowed-repos: 42 or a mixed array failed validation because validateReposScope still saw the original value and returned an explicit type error. Now parseGitHubReposScope converts every unsupported shape into an empty slice, and validateGitHubGuardPolicy treats that as if the field were omitted (hasRepos := github.AllowedRepos != nil is false for an empty slice). That changes invalid policy input into permissive defaulting:
tools:
github:
allowed-repos: 42
min-integrity: approvedWith the new parser, that reaches:
config.AllowedRepos = GitHubReposScope{}
hasRepos := github.AllowedRepos != nil // false
github.AllowedRepos = GitHubReposScope{"all"}So a bad config no longer produces a schema/validation error; it broadens access to all. Please preserve parse/validation failure for unsupported scalar types and mixed/non-string arrays, and add a regression test around invalid allowed-repos input going through NewTools(...) + validateGitHubGuardPolicy(...).
There was a problem hiding this comment.
Addressed by ad17985 and covered again in c60b4f2: unsupported scalar and mixed-array scopes fail before defaulting, with min-integrity: approved regression coverage through NewTools validation and ParseToolsConfig.
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32874927562
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in ad17985. Malformed scalar and mixed-array repository scopes now return contextual parse errors through |
|
Please do one focused follow-up pass:
Run: https://github.com/github/gh-aw/actions/runs/32891822013
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
GitHubReposScopeusedany, forcing consumers to handle scalar strings and YAML-parsed arrays independently. This change normalizes both supported YAML forms into a typed string slice.Type
GitHubReposScopeas[]string.Consumers
Both forms now produce:
pr-sous-chef run: https://github.com/github/gh-aw/actions/runs/32874927562