Skip to content

Conversation

Copy link
Contributor

Copilot AI commented Nov 9, 2025

Two different ValidatePermissions functions existed with incompatible signatures, causing navigation confusion:

  • pkg/workflow/imports.go:161 - Compiler method validating permissions from imported files
  • pkg/workflow/permissions_validator.go:92 - Standalone function validating permissions against GitHub MCP toolsets

Changes

  • Renamed Compiler.ValidatePermissions()ValidateIncludedPermissions() in pkg/workflow/imports.go
  • Updated caller in pkg/workflow/compiler.go:676
  • Updated test function TestValidatePermissionsTestValidateIncludedPermissions
  • Added cross-reference documentation to both functions explaining distinct purposes

Usage

// General permission validation against GitHub MCP toolsets
result := ValidatePermissions(permissions, githubTool)

// Permission validation for imported/included workflow files
err := compiler.ValidateIncludedPermissions(topPermissionsYAML, importedPermissionsJSON)

The renamed function clarifies its specific role in the imports workflow, while the standalone validator remains the canonical general-purpose validator.

Original prompt

This section details on the original issue you should resolve

<issue_title>[task] Resolve ValidatePermissions naming conflict between imports.go and permissions_validator.go</issue_title>
<issue_description>## Objective
Rename conflicting ValidatePermissions functions to clarify their distinct purposes and prevent confusion.

Context

Two different ValidatePermissions functions exist with different signatures:

  • pkg/workflow/imports.go:347 - Part of Compiler, validates permissions from included files
  • pkg/workflow/permissions_validator.go:92 - Standalone function using PermissionsValidator

This creates confusion when calling validation functions and makes the codebase harder to navigate.

Part of issue #3435.

Approach

1. Analyze function purposes

Review both functions to understand their specific responsibilities:

  • imports.go version: Validates permissions when merging included files
  • permissions_validator.go version: General permission validation

2. Rename for clarity

Rename the imports.go function to ValidateIncludedPermissions to reflect its specific purpose of validating permissions from included files.

Keep the permissions_validator.go function as ValidatePermissions since it's the general-purpose validator.

3. Update all callers

  • Find all references to the renamed function
  • Update function calls to use the new name
  • Update any related documentation or comments

4. Add documentation

Add clear comments explaining:

  • When to use ValidatePermissions (general validation)
  • When to use ValidateIncludedPermissions (when merging includes)

Files to Modify

  • pkg/workflow/imports.go - Rename function from ValidatePermissions to ValidateIncludedPermissions
  • All files that call imports.go's ValidatePermissions function
  • Add documentation comments to both functions

Acceptance Criteria

AI generated by Plan Command for #3435</issue_description>

Comments on the Issue (you are @copilot in this section)


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

- Renamed Compiler.ValidatePermissions() to ValidateIncludedPermissions() in pkg/workflow/imports.go
- Updated caller in pkg/workflow/compiler.go
- Updated test function name in pkg/workflow/permissions_import_test.go
- Added comprehensive documentation to both functions explaining their distinct purposes
- All tests pass (make test-unit)
- Build succeeds (make build)

Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot AI changed the title [WIP] Rename ValidatePermissions functions to clarify purposes Rename ValidatePermissions to ValidateIncludedPermissions in imports.go Nov 9, 2025
Copilot AI requested a review from pelikhan November 9, 2025 19:25
@pelikhan pelikhan marked this pull request as ready for review November 9, 2025 19:32
Copilot AI review requested due to automatic review settings November 9, 2025 19:32
@pelikhan pelikhan merged commit 051ab44 into main Nov 9, 2025
74 checks passed
@pelikhan pelikhan deleted the copilot/rename-validate-permissions-functions branch November 9, 2025 19:34
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR renames a function to better distinguish between two different permission validation use cases. The function ValidatePermissions in imports.go has been renamed to ValidateIncludedPermissions to clarify that it specifically validates permissions when merging included/imported workflow files, distinguishing it from the general-purpose ValidatePermissions function in permissions_validator.go that validates permissions against GitHub MCP toolsets.

Key changes:

  • Renamed Compiler.ValidatePermissions() to Compiler.ValidateIncludedPermissions() in imports.go
  • Updated all call sites to use the new function name
  • Added comprehensive documentation explaining when to use each validation function

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
pkg/workflow/imports.go Renamed method from ValidatePermissions to ValidateIncludedPermissions and enhanced documentation to clarify its specific use case for validating imported workflow permissions
pkg/workflow/permissions_validator.go Enhanced documentation for ValidatePermissions function to clarify it's for general MCP toolset validation and distinguish it from the renamed function
pkg/workflow/compiler.go Updated function call from ValidatePermissions to ValidateIncludedPermissions
pkg/workflow/permissions_import_test.go Updated test function name and all error messages to reflect the renamed function

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[task] Resolve ValidatePermissions naming conflict between imports.go and permissions_validator.go

2 participants