refactor: consolidate hasCopilotRequestsWritePermission frontmatter adapter into workflow package#44886
Conversation
Add HasCopilotRequestsWriteFromFrontmatter to pkg/workflow as the single exported predicate for frontmatter-based permission checks. Remove the duplicate local adapter from pkg/cli/workflow_secrets.go and replace its call site with workflow.HasCopilotRequestsWriteFromFrontmatter. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Reference the unexported counterpart function explicitly to make it clear both functions live in the same file. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
✅ PR Code Quality Reviewer completed the code quality review. |
|
✅ Test Quality Sentinel completed test quality analysis. No test files were added or modified in this PR. Test Quality Sentinel skipped. |
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. No ADR enforcement needed: PR does not have the 'implementation' label and has ≤100 new lines of code in business logic directories (17 lines added across 2 files). |
There was a problem hiding this comment.
Pull request overview
Consolidates Copilot permission parsing in pkg/workflow without changing behavior.
Changes:
- Adds an exported frontmatter permission helper.
- Replaces and removes the duplicate CLI adapter.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/permissions_operations.go |
Adds the centralized frontmatter adapter. |
pkg/cli/workflow_secrets.go |
Uses the workflow helper and removes duplicate logic. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 0
- Review effort level: Medium
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅ |
There was a problem hiding this comment.
Clean consolidation — the new exported function is a faithful lift of the deleted private one, and co-locating both entry points in pkg/workflow is the right call. No blocking issues.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · 9.8 AIC · ⌖ 5.13 AIC · ⊞ 4.8K
There was a problem hiding this comment.
Clean behaviour-preserving refactor. The extracted function is logically identical to the removed private adapter; nil guard and conversion logic are preserved. No issues found.
🔎 Code quality review by PR Code Quality Reviewer · 26.5 AIC · ⌖ 4.27 AIC · ⊞ 5.4K
Comment /review to run again
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design — this is a clean, behaviour-preserving consolidation.
📋 Highlights
Positive
- ✅ Eliminates duplication of the frontmatter-to-Permissions conversion logic
- ✅ Nil and missing-key guards are correctly preserved in the moved implementation
- ✅ Co-location with the unexported
hasCopilotRequestsWritePermission(*WorkflowData)adapter makes both entry points visible side-by-side — good discoverability - ✅ Chaining
NewPermissionsParserFromValue(...).ToPermissions().HasCopilotRequestsWrite()is idiomatic and equivalent to the previous two-step form
Minor observation
The doc comment on HasCopilotRequestsWriteFromFrontmatter is verbose relative to the function's simplicity. A shorter form (e.g. // HasCopilotRequestsWriteFromFrontmatter reports whether frontmatter declares copilot-requests: write.) would be more in line with Go convention, but this is not blocking.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · 14.8 AIC · ⌖ 4.37 AIC · ⊞ 6.6K
Comment /matt to run again
|
🎉 This pull request is included in a new release. Release: |
Two same-named
hasCopilotRequestsWritePermissionfunctions lived in separate packages with different input types, both delegating to(*Permissions).HasCopilotRequestsWrite(). The frontmatter-based adapter inpkg/cliduplicated the frontmatter-to-*Permissionsconversion logic that belongs inpkg/workflow.Changes
pkg/workflow/permissions_operations.go— Adds exportedHasCopilotRequestsWriteFromFrontmatter(frontmatter map[string]any) bool, co-located with the existinghasCopilotRequestsWritePermission(*WorkflowData)adapter so both entry points are visible side-by-side.pkg/cli/workflow_secrets.go— Removes the localhasCopilotRequestsWritePermissionadapter and replaces its call site withworkflow.HasCopilotRequestsWriteFromFrontmatter.Behaviour-preserving; the conversion logic is identical.