Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage
Part of a 3-PR cluster refactoring safe-output config parsing. Recommend reviewing all three together for consistency before merging.
|
|
✅ Ponytail Reviewer completed successfully! Lean already. Ship.
|
|
❌ Design Decision Gate 🏗️ failed during design decision gate check. Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details.
|
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ PR Code Quality Reviewer completed the code quality review.
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
There was a problem hiding this comment.
Pull request overview
Centralizes safe-output target parsing while preserving handler-specific wildcard and expression policies.
Changes:
- Adds an options-based shared target parser.
- Refactors eight safe-output handlers to use it.
- Expands parser and cross-repository behavior tests.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/safe_outputs_parser.go |
Adds the shared configurable parser. |
pkg/workflow/config_helpers.go |
Delegates repository validation to the shared parser. |
pkg/workflow/update_project.go |
Uses shared cross-repository parsing. |
pkg/workflow/submit_pr_review.go |
Consolidates review target parsing. |
pkg/workflow/reply_to_pr_review_comment.go |
Consolidates reply target parsing. |
pkg/workflow/create_pr_review_comment.go |
Consolidates comment target parsing. |
pkg/workflow/push_to_pull_request_branch.go |
Preserves wildcard and expression support. |
pkg/workflow/link_sub_issue.go |
Preserves expression-aware repository parsing. |
pkg/workflow/dispatch_workflow.go |
Centralizes dispatch repository parsing. |
pkg/workflow/create_code_scanning_alert.go |
Centralizes alert repository parsing. |
pkg/workflow/config_parsing_helpers_test.go |
Tests parser options and handler behavior. |
pkg/workflow/safe_outputs_cross_repo_config_test.go |
Verifies code-scanning wildcard preservation. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 12/12 changed files
- Comments generated: 0
- Review effort level: Balanced
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.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Request changes
This refactor introduced at least one behavior regression in safe-output cross-repo parsing: create-pull-request-review-comment no longer preserves expression-based allowed-repos, so workflows that relied on ${{ ... }} there will quietly lose their repo allowlist at compile time.
🔎 Code quality review by PR Code Quality Reviewer · gpt54 · 51.5 AIC · ⌖ 7.69 AIC · ⊞ 7K
Comment /review to run again
| targetConfig, isInvalid := parseSafeOutputTargetConfig(configMap, createPRReviewCommentLog, safeOutputTargetConfigOptions{ | ||
| parseTarget: true, | ||
| parseTargetRepo: true, | ||
| parseAllowedRepos: true, |
There was a problem hiding this comment.
This refactor drops expression support for allowed-repos on create-pull-request-review-comment, because the parser now uses ParseStringArrayFromConfig while the handler still expects to pass templated values through to runtime. Any workflow that previously fed allowed-repos: ${{ ... }} into review comments will silently lose its repo allowlist and stop targeting cross-repo PRs correctly.
💡 Why this is a regression and how to fix it
create_pull_request_review_comment is still emitted with AddStringSlice("allowed_repos", c.AllowedRepos), so it only works when c.AllowedRepos preserves the raw expression. The new shared helper only preserves expressions when allowAllowedReposExpression is enabled, but this caller does not opt in.
Use the expression-aware path here as well, or add a regression test that proves ${{ inputs['allowed-repos'] }} survives parsing and reaches the generated handler config.
targetConfig, isInvalid := parseSafeOutputTargetConfig(configMap, createPRReviewCommentLog, safeOutputTargetConfigOptions{
parseTarget: true,
parseTargetRepo: true,
parseAllowedRepos: true,
allowAllowedReposExpression: true,
})There was a problem hiding this comment.
Fixed in a0aebb4: create-pull-request-review-comment now opts into expression-aware allowed-repos parsing, with regression coverage proving ${{ inputs['allowed-repos'] }} is preserved.
There was a problem hiding this comment.
Clean refactoring that successfully extracts shared safe-output target parsing into a single parseSafeOutputTargetConfig helper with configurable options. The approach is sound — good use of options struct to control parse behavior per handler. One stale comment found (inline).
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 26.3 AIC · ⌖ 8.82 AIC · ⊞ 5.7K
| allowAllowedReposExpression: true, | ||
| }) | ||
| if isInvalid { | ||
| return nil // Invalid configuration (e.g., wildcard target-repo), return nil to cause validation error |
There was a problem hiding this comment.
The inline comment says "Invalid configuration (e.g., wildcard target-repo)", but allowTargetRepoWildcard: true is set in the options above, so wildcards are allowed for this handler. The example in the comment is now misleading and will confuse future readers.
Suggested fix:
return nil // Invalid configuration, return nil to cause validation error@copilot please address this.
There was a problem hiding this comment.
Fixed in 619a311: removed the misleading wildcard example from the invalid-configuration comment in pkg/workflow/link_sub_issue.go.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — two small follow-ups; no blocking issues.
📋 Key Themes & Highlights
Key Themes
- Struct inconsistency:
CreatePullRequestReviewCommentsConfigassignsTarget/TargetRepoSlug/AllowedReposindividually rather than embeddingSafeOutputTargetConfiglike its sibling structs now do. - Undocumented policy gap:
create_code_scanning_alertsilently dropsallowAllowedReposExpressionwhile similar handlers enable it — the intent isn't documented.
Positive Highlights
- ✅ Clean options-struct pattern — explicit, testable, no hidden globals
- ✅
nilguard ondebugLogis a small but good defensive detail - ✅ Test coverage is solid: option-by-option cases, wildcard, expression-aware paths all covered
- ✅
ParseTargetConfigpreserved as a thin adapter — existing callers unaffected
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 51.5 AIC · ⌖ 10.1 AIC · ⊞ 7.8K
Comment /matt to run again
Comments that could not be inline-anchored
pkg/workflow/create_pr_review_comment.go:16
[/codebase-design] CreatePullRequestReviewCommentsConfig still expands Target, TargetRepoSlug, and AllowedRepos as individual fields instead of embedding SafeOutputTargetConfig — unlike the sibling structs (SubmitPullRequestReviewConfig, ReplyToPRReviewCommentConfig) that this PR already migrated to the embedded form.
<details>
<summary>💡 Suggested change</summary>
Embed SafeOutputTargetConfig in the struct definition:
type CreatePullRequestReviewCommentsConfig str…
</details>
<details><summary>pkg/workflow/create_code_scanning_alert.go:164</summary>
**[/codebase-design]** `create_code_scanning_alert.go` no longer passes `allowAllowedReposExpression: true`, but the pre-existing `ParseStringArrayFromConfig` call it replaced did not support expressions either — so this is a pre-existing limitation, not a regression. However, it is now inconsistent with other handlers that accept security-sensitive configs (e.g. `dispatch_workflow`, `push_to_pull_request_branch`) which do allow expression-based `allowed-repos`. Worth documenting or aligning in…
</details>
|
@copilot Please address the latest blocking review feedback on this PR and then run the Current blocker to resolve:
Failing checks on the current head: A branch refresh was attempted by sous-chef as well.
|
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 |
|
@copilot A recent author update landed on this PR, but there is still no maintainer-visible resolution on the blocking review. Please refresh the branch if needed, re-run the affected checks, and then run the Current maintainer-visible blocker:
A branch refresh was attempted by sous-chef as well.
|
Confirmed on current head |
|
@copilot A recent author update landed on this PR, but the blocking Current maintainer-visible blocker:
|
Confirmed on current head I refreshed against |
|
Triage: category= Part of the B1-safeoutput-refactor batch (with #53840, #53838). CHANGES_REQUESTED outstanding and mergeable_state=blocked; active resolution thread with gh-aw-bot. Batch-review alongside sibling PRs once threads are resolved.
|
Several safe-output config parsers duplicated the same cross-repo parsing flow for
target,target-repo, andallowed-repos. This centralizes that behavior so wildcard and expression handling stay consistent across handlers.parseSafeOutputTargetConfighelper.allowed-repos.Handler refactor
Focused coverage
allowed-reposand wildcard preservation.run: https://github.com/github/gh-aw/actions/runs/32204573204> Generated by 👨🍳 PR Sous Chef · gpt54 · 25.9 AIC · ⌖ 8.32 AIC · ⊞ 9.3K · ◷
Run: https://github.com/github/gh-aw/actions/runs/32210439073> Generated by 👨🍳 PR Sous Chef · gpt54 · 25.8 AIC · ⌖ 8.18 AIC · ⊞ 9.3K · ◷