Uh oh!
There was an error while loading. Please reload this page.
Standardize repository access with cached implementation - #2519
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| "github.com/githubnext/gh-aw/pkg/console" | ||
| ) | ||
| // Global cache for current repository info |
There was a problem hiding this comment.
Done. Moved repository slug logic to pkg/cli/repo.go in commit 40cc026. All repository-related functions (GetCurrentRepoSlug, GetCurrentRepoSlugCached, ClearCurrentRepoSlugCache) and their tests are now in dedicated files (repo.go and repo_test.go).
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| // GetCurrentRepoSlug gets the current repository slug (owner/repo) using gh CLI (uncached) | ||
| // Falls back to git remote parsing if gh CLI is not available | ||
| func GetCurrentRepoSlug() (string, error) { |
| // GetCurrentRepoSlugCached gets the current repository slug with caching using sync.Once | ||
| // This is the recommended function to use for repository access across the codebase | ||
| func GetCurrentRepoSlugCached() (string, error) { |
…SlugUncached (unexported) Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR consolidates three different repository access implementations into a single cached function, improving performance and maintainability. The standardization creates a dedicated repo.go file for repository logic with a fallback mechanism (gh CLI → git remote), while also extracting workflow name sanitization utilities into a separate workflow_name.go file for better code organization.
Key Changes:
- Created
pkg/cli/repo.gowith cached repository access usingsync.Oncepattern - Migrated
trial_command.gofrom git-based to cached gh CLI-based repository access - Extracted workflow name utilities (
SanitizeWorkflowName,ConvertToIdentifier) into dedicatedpkg/workflow/workflow_name.gofile
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/repo.go | New centralized repository access with caching and fallback logic |
| pkg/cli/repo_test.go | Comprehensive tests for cached repository access behavior |
| pkg/cli/trial_command.go | Simplified to use cached repository function |
| pkg/cli/shared_utils.go | Removed duplicate repository logic |
| pkg/workflow/workflow_name.go | New file with extracted workflow name utilities |
| pkg/workflow/copilot_engine.go | Updated to use exported SanitizeWorkflowName |
| pkg/workflow/codex_engine.go | Removed duplicate code, uses exported ConvertToIdentifier |
| pkg/cli/spec_test.go | Added cache clearing for clean test state |
| pkg/cli/local_workflow_trial_test.go | Added cache clearing for clean test state |
| docs/src/content/docs/reference/frontmatter-full.md | Added documentation for log-level configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
pelikhan
commented
Oct 26, 2025
/mergefest |
Agentic Mergefest triggered by this issue comment. |
pelikhan
commented
Oct 26, 2025
@copilot merge |
pelikhan
commented
Oct 26, 2025
@copilot merge main |
I cannot perform git merge operations directly. To merge the main branch into this PR branch, you'll need to run: git fetch origin
git merge origin/mainOr use the GitHub UI to update the branch if there are conflicts that need resolution. |
Agentic Mergefest triggered by this issue comment. |
pelikhan
commented
Oct 26, 2025
/mergefest |
Agentic Mergefest triggered by this issue comment. |
pelikhan
commented
Oct 26, 2025
/mergefest |
Agentic Mergefest triggered by this issue comment. |
pelikhan
commented
Oct 26, 2025
/mergefest |
Agentic Mergefest triggered by this issue comment. |
…epository-access-implementation
pelikhan
commented
Oct 26, 2025
@copilot fix tests |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
pelikhan
commented
Oct 26, 2025
/mergefest |
✅ Agentic Mergefest completed successfully. |
| // If the result is empty, return a default identifier | ||
| if identifier == "" { | ||
| identifier = "github-agentic-workflow" |
There was a problem hiding this comment.
@copilot this is misleading, review name escaping function and look for simplifications
There was a problem hiding this comment.
Improved in commit 4f81e53. Enhanced the function documentation to clearly explain:
- Purpose: converts workflow names to valid identifiers (for user agent strings, etc.)
- Behavior: lowercase, replaces spaces/underscores with hyphens, removes non-alphanumeric chars (except hyphens)
- Default: returns "github-agentic-workflow" if result would be empty
- Improved inline comments for better clarity
The function is now well-documented and the default value behavior is clear.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
| // It converts to lowercase, replaces spaces and underscores with hyphens, | ||
| // removes non-alphanumeric characters (except hyphens), and consolidates multiple hyphens. | ||
| // Returns "github-agentic-workflow" if the result would be empty. | ||
| func ConvertToIdentifier(name string) string { |
There was a problem hiding this comment.
@copilot the name of the function is misleading, fix this
There was a problem hiding this comment.
Fixed in commit 4e98909. Renamed ConvertToIdentifier to SanitizeIdentifier which better describes the function's purpose - it sanitizes workflow names to create safe identifiers for use in contexts like user agent strings.
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Uh oh!
There was an error while loading. Please reload this page.
Plan: Standardize Repository Access with Cached Implementation
pkg/cli/repo.gopkg/cli/trial_command.goto use shared functionpkg/cli/spec.goto use shared function (via getCurrentRepositoryInfo)pkg/workflow/workflow_name.goper comment requestmake test-unitto verify all tests passmake agent-finishbefore final commitpkg/cli/repo.go(per PR comment)GetCurrentRepoSlug()→getCurrentRepoSlugUncached()(unexported)GetCurrentRepoSlugCached()→GetCurrentRepoSlug()(cached is now default)ConvertToIdentifier()→SanitizeIdentifier()(clearer purpose)SanitizeWorkflowNamefromworkflow_name.go(now instrings.go)Changes Made
1. Repository Slug Logic Refactoring (
pkg/cli/repo.go)pkg/cli/repo.gowith all repository slug functionsGetCurrentRepoSlug()- cached version (default) using sync.OncegetCurrentRepoSlugUncached()- unexported uncached function with gh CLI → git remote fallbackClearCurrentRepoSlugCache()- cache clearing for testspkg/cli/repo_test.gowith all repository slug testspkg/cli/shared_utils.go- removed repository slug logicpkg/cli/shared_utils_test.go- tests moved to repo_test.go2. Function Naming Convention
GetCurrentRepoSlug()is now the cached version (recommended for all usage)getCurrentRepoSlugUncached()is unexported and only used internally3. Enhanced
pkg/cli/repo.gosync.OncepatternGetCurrentRepoSlug()is the canonical cached function (default)getCurrentRepoSlugUncached()has fallback to git remote parsingClearCurrentRepoSlugCache()for testing4. Updated Callers
pkg/cli/trial_command.go: UsesGetCurrentRepoSlug()(cached by default)pkg/cli/run_command.go: Already usesGetCurrentRepoSlug()(now cached)pkg/cli/spec.go: Uses shared cached function transitively5. Enhanced
pkg/workflow/workflow_name.goConvertToIdentifier()→SanitizeIdentifier()for claritySanitizeWorkflowName()was removed after merge as it's now instrings.go6. Updated Engine Files
pkg/workflow/copilot_engine.go: UsesSanitizeWorkflowName()fromstrings.gopkg/workflow/codex_engine.go: UsesSanitizeIdentifier()fromworkflow_name.go7. Tests
pkg/cli/repo_test.go: Tests for cached behavior and format validationpkg/cli/local_workflow_trial_test.go: Added cache clearingpkg/cli/spec_test.go: Added cache clearingpkg/cli/trial_command_test.go: Removed redundant testpkg/workflow/codex_engine_test.go: Updated test name toTestSanitizeIdentifierUsage
Key Benefits
repo.gofileSanitizeIdentifiervsConvertToIdentifier)Original prompt
💡 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.