Conversation
…nctions Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Refactor duplicate and outlier functions in pkg/workflow
refactor(workflow): Priority 1 semantic clustering — shared mount validation, copilot function relocation
Feb 22, 2026
Contributor
|
@copilot add tests |
…x, generateCopilotSessionFileCopyStep Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
Added in commit 5ae9713:
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
pelikhan
deleted the
copilot/refactor-semantic-function-clustering-6dcb86e4-d38e-4ade-8b9b-9856d4d1cd55
branch
Contributor
There was a problem hiding this comment.
Pull request overview
This PR implements Priority 1 refactoring items from a semantic function clustering analysis, focusing on extracting shared mount validation logic and relocating copilot execution helper functions to improve code organization.
Changes:
- Extracted duplicated mount string validation logic into a shared
validateMountStringFormat()helper function - Relocated two copilot execution helper functions (
extractAddDirPaths()andgenerateCopilotSessionFileCopyStep()) tocopilot_engine_execution.gowhere they semantically belong - Added comprehensive test coverage for the new validation helper and MCP mount validation
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| pkg/workflow/validation_helpers.go | Added shared validateMountStringFormat() helper to parse and validate "source:dest:mode" mount strings |
| pkg/workflow/validation_helpers_test.go | Added comprehensive tests for mount format validation including edge cases |
| pkg/workflow/sandbox_validation.go | Refactored to use shared validation helper; removed duplicate validation logic and unused strings import |
| pkg/workflow/mcp_config_validation.go | Refactored to use shared validation helper; properly distinguishes format vs mode errors |
| pkg/workflow/mcp_config_validation_test.go | New test file with 12 test cases covering MCP mount validation scenarios |
| pkg/workflow/copilot_engine.go | Removed extractAddDirPaths() function (moved to execution file) |
| pkg/workflow/copilot_engine_execution.go | Added relocated extractAddDirPaths() and generateCopilotSessionFileCopyStep() functions; removed orphaned comment |
| pkg/workflow/copilot_logs.go | Removed generateCopilotSessionFileCopyStep() function (moved to execution file) |
| pkg/workflow/copilot_engine_test.go | Added test for generateCopilotSessionFileCopyStep() to verify step generation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| // validateMountStringFormat parses a mount string and validates its basic format. | ||
| // Expected format: "source:destination:mode" where mode is "ro" or "rw". | ||
| // Returns (source, dest, mode, nil) on success, or ("", "", "", error) on failure. |
There was a problem hiding this comment.
The documentation comment is inaccurate. It states "Returns (source, dest, mode, nil) on success, or ("", "", "", error) on failure", but this doesn't describe the actual behavior correctly. The function returns empty strings for all values only when there's a format error (wrong number of parts), but returns the actual source, dest, and mode values when there's a mode validation error. The comment should be updated to clarify this distinction, for example: "Returns (source, dest, mode, nil) on success. On format errors (wrong number of parts), returns ("", "", "", error). On mode validation errors, returns (source, dest, invalid_mode, error) so callers can provide better error messages."
Suggested change
| // Returns (source, dest, mode, nil) on success, or ("", "", "", error) on failure. | |
| // Returns (source, dest, mode, nil) on success. | |
| // On format errors (wrong number of parts), returns ("", "", "", error). | |
| // On mode validation errors, returns (source, dest, invalid_mode, error) so callers can provide better error messages. |
This was referenced Feb 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses the three Priority 1 items from the semantic function clustering analysis: extract shared mount validation logic, and relocate two copilot execution helpers to their call sites.
Changes
validateMountStringFormat()→validation_helpers.goNew shared primitive that parses
source:destination:modeformat (3-parts check +ro/rwmode check). Both consumers delegate core validation here and wrap errors in their own context-appropriate types:validateMountsSyntax()(sandbox_validation.go) — wraps asNewValidationError; also drops now-unusedstringsimportvalidateMCPMountsSyntax()(mcp_config_validation.go) — wraps asfmt.Errorf; usessource == "" && dest == "" && mode == ""to correctly distinguish format vs. mode errors (edge case: mount with empty mode string like/foo:/bar:would otherwise misclassify)extractAddDirPaths()moved tocopilot_engine_execution.goWas in
copilot_engine.gobut only called fromcopilot_engine_execution.go.generateCopilotSessionFileCopyStep()moved tocopilot_engine_execution.goWas in
copilot_logs.go; grouped with other execution step generators.Tests
TestValidateMountStringFormat(6 cases,validation_helpers_test.go) — covers valid mounts, format errors where all return values are empty (wrong number of parts), and mode errors where source/dest are returned alongside the invalid mode value (including the empty-mode edge case/foo:/bar:).TestValidateMCPMountsSyntax(12 cases, newmcp_config_validation_test.go) — covers[]stringand[]anyinputs, wrong type, format/mode errors, error messages including tool name and mount index, and silent skipping of non-string[]anyitems.TestGenerateCopilotSessionFileCopyStep(copilot_engine_test.go) — verifies the relocated function produces a step with the correct name,always()condition, session-state source and gh-aw logs destination directories, andcontinue-on-error: true.Original prompt
This section details on the original issue you should resolve
<issue_title>[refactor] Semantic function clustering: duplicates and outliers in pkg/workflow</issue_title>
<issue_description>Automated semantic analysis of all non-test Go files in
pkg/workflow/(240+ files) and utility packages (pkg/stringutil,pkg/sliceutil, etc.) identified several concrete refactoring opportunities through function clustering and duplicate detection.Overview
Critical Issues
Issue 1: Duplicate Mount Syntax Validation
Two functions implement nearly identical mount string validation (
source:dest:modeformat) in separate files:pkg/workflow/sandbox_validation.go→validateMountsSyntax()pkg/workflow/mcp_config_validation.go→validateMCPMountsSyntax()Both split on
:, check for exactly 3 parts, and validate thatmodeis"ro"or"rw". The only differences are error types (NewValidationErrorvsfmt.Errorf) and that the sandbox version also validates non-empty source/dest fields.Recommendation: Extract a shared
validateMountFormat(mounts []string, toolName string) errorfunction intovalidation_helpers.goor a newmounts_validation.go, then call it from both files with appropriate error wrapping.Issue 2: Near-Identical missing_data.go and missing_tool.go
pkg/workflow/missing_data.goandpkg/workflow/missing_tool.goare structurally identical (~164 lines each, ~250 duplicated lines):View duplicate struct definitions
Both
parseMissingDataConfig()/parseMissingToolConfig()andbuildCreateOutputMissingDataJob()/buildCreateOutputMissingToolJob()follow exactly the same pattern with only the output type name and environment variable prefix differing (e.g.,GH_AW_MISSING_DATA_MAXvsGH_AW_MISSING_TOOL_MAX).Recommendation: Consider a shared
BaseMissingOutputConfigtype and a genericbuildCreateOutputMissingJob(outputType string, config BaseMissingOutputConfig)builder, reducing ~250 lines to ~80 lines of shared code.noop.gofollows the same outer parse pattern and could share the same boilerplate helper.Issue 3: Safe Output Handler Parse Function Boilerplate (11 files)
The following files all implement the same three-part pattern — a config struct embedding
BaseSafeOutputConfig+SafeOutputTargetConfig, aparseXxxConfig()method on*Compiler, and a build function:add_comment.go,add_labels.go,add_reviewer.go,assign_milestone.go,assign_to_agent.go,assign_to_user.go,hide_comment.go,link_sub_issue.go,remove_labels.go,reply_to_pr_review_comment.go,resolve_pr_review_thread.goView duplicated parse function pattern
The
AssignToUserConfigandUnassignFromUserConfigstructs are also nearly identical: