Introduce a specialized agent to migrate commands from PHP to Go#16
Introduce a specialized agent to migrate commands from PHP to Go#16akalipetis wants to merge 8 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Introduces infrastructure and guidance to support migrating legacy PHP (Symfony Console) commands to native Go (Cobra), and ports the project:list command along with supporting API/table utilities.
Changes:
- Adds a
PHP to Go Command Migrator Specialist Agentguide and Copilot setup workflow to streamline migrations. - Implements a legacy-style table output formatter with wrapping support.
- Adds Go API client methods for listing projects via user grants and resolving signed HAL ref links; adds the Go
project:listcommand and registers it.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/tableoutput/table.go | New table rendering package with terminal-width handling and CSV/TSV/plain output. |
| internal/tableoutput/table_test.go | Unit tests for table rendering and wrapping behavior. |
| internal/api/users.go | Adds GetMyUserID() helper for current-user lookups. |
| internal/api/refs.go | Adds helpers to extract/use signed HAL ref links for projects/orgs. |
| internal/api/project.go | Adds GetMyProjects() using extended-access grants + HAL refs expansion. |
| commands/project_list.go | New native Go implementation of project:list (filters/sort/output). |
| commands/root.go | Registers the new project:list command with the root command. |
| README.md | Documents the migration agent and migration workflow pointers. |
| CLAUDE.md | Updates repo guidance and documents new migration/table/API components. |
| .github/workflows/copilot-setup-steps.yml | Adds GitHub Copilot coding agent setup steps workflow. |
| .github/agents/migrator-php-to-go.agent.md | Adds the specialized migration agent instructions and checklists. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| cmd.Flags().Int("refresh", 1, "Whether to refresh the list") | ||
| cmd.Flags().String("sort", "title", "A property to sort by") | ||
| cmd.Flags().Bool("reverse", false, "Sort in reverse (descending) order") | ||
| cmd.Flags().Int("page", 0, "Page number. This enables pagination.") | ||
| cmd.Flags().IntP("count", "c", 0, "The number of projects to display per page. Use 0 to disable pagination.") |
There was a problem hiding this comment.
The flags --refresh, --page, and --count are defined here but are never read/used in runProjectList. In the legacy implementation these options affect caching and pagination, so leaving them unused is misleading and changes behavior; either implement them (including page-count interactions) or remove them from the command interface.
| cmd.Flags().Int("refresh", 1, "Whether to refresh the list") | |
| cmd.Flags().String("sort", "title", "A property to sort by") | |
| cmd.Flags().Bool("reverse", false, "Sort in reverse (descending) order") | |
| cmd.Flags().Int("page", 0, "Page number. This enables pagination.") | |
| cmd.Flags().IntP("count", "c", 0, "The number of projects to display per page. Use 0 to disable pagination.") | |
| cmd.Flags().String("sort", "title", "A property to sort by") | |
| cmd.Flags().Bool("reverse", false, "Sort in reverse (descending) order") |
| func renderOutput(cmd *cobra.Command, table *tableoutput.Table) error { | ||
| format, _ := cmd.Flags().GetString("format") | ||
| noHeader, _ := cmd.Flags().GetBool("no-header") | ||
| w := cmd.OutOrStdout() | ||
|
|
||
| switch format { | ||
| case "csv": | ||
| return table.RenderCSV(w, noHeader) | ||
| case "tsv", "plain": | ||
| return table.RenderPlain(w) | ||
| default: // table | ||
| return table.RenderTable(w) | ||
| } |
There was a problem hiding this comment.
--no-header is parsed but only applied to CSV output. For table/plain/tsv formats the header is always printed, which diverges from the legacy Table behavior (where --no-header removes headers for all formats). Consider extending tableoutput to support rendering without a header, and have renderOutput honor --no-header for all formats.
| if cnf.API.EnableOrganizations { | ||
| result = slices.DeleteFunc(result, func(p *api.ProjectInfo) bool { | ||
| if p.OrganizationRef != nil { | ||
| return p.OrganizationRef.OwnerID != myUserID | ||
| } | ||
| return true | ||
| }) | ||
| } |
There was a problem hiding this comment.
The --my filter is only applied when organizations are enabled and OrganizationRef is present; otherwise it leaves the result unfiltered (or filters everything if orgs are enabled but OrganizationRef is nil). In the legacy command, --my falls back to comparing the project's owner_id when org info isn't available. To match that behavior, include OwnerID in ProjectInfo and use it as a fallback when OrganizationRef is nil or organizations are disabled.
| if cnf.API.EnableOrganizations { | |
| result = slices.DeleteFunc(result, func(p *api.ProjectInfo) bool { | |
| if p.OrganizationRef != nil { | |
| return p.OrganizationRef.OwnerID != myUserID | |
| } | |
| return true | |
| }) | |
| } | |
| result = slices.DeleteFunc(result, func(p *api.ProjectInfo) bool { | |
| // When organizations are enabled and the project has an organization, | |
| // use the organization's owner as the source of truth. | |
| if cnf.API.EnableOrganizations && p.OrganizationRef != nil { | |
| return p.OrganizationRef.OwnerID != myUserID | |
| } | |
| // Otherwise, fall back to the project's direct OwnerID. | |
| return p.OwnerID != myUserID | |
| }) |
| cmd.Flags().Bool("pipe", false, "Output a simple list of project IDs. Disables pagination.") | ||
| cmd.Flags().String("region", "", "Filter by region (exact match)") | ||
| cmd.Flags().String("title", "", "Filter by title (case-insensitive search)") | ||
| cmd.Flags().Bool("my", false, "Display only the projects you own") |
There was a problem hiding this comment.
The legacy command supports a deprecated --host option as an alias for --region (and emits a deprecation warning). This Go implementation omits --host entirely, which can break existing scripts. Consider adding a hidden --host flag mapped to --region (with a deprecation notice) to preserve CLI compatibility.
| if err == nil && myUserID != "" { | ||
| if cnf.API.EnableOrganizations { | ||
| result = slices.DeleteFunc(result, func(p *api.ProjectInfo) bool { | ||
| if p.OrganizationRef != nil { | ||
| return p.OrganizationRef.OwnerID != myUserID | ||
| } | ||
| return true | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
filterProjects ignores errors from apiClient.GetMyUserID() when --my is set (it silently skips filtering). In the legacy implementation, failing to determine the current user is an error for --my. Consider returning an error from filterProjects (or handling it in runProjectList) so the command fails loudly when ownership filtering can't be computed.
| if err == nil && myUserID != "" { | |
| if cnf.API.EnableOrganizations { | |
| result = slices.DeleteFunc(result, func(p *api.ProjectInfo) bool { | |
| if p.OrganizationRef != nil { | |
| return p.OrganizationRef.OwnerID != myUserID | |
| } | |
| return true | |
| }) | |
| } | |
| } | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to determine current user for --my filter: %w", err) | |
| } | |
| if myUserID == "" { | |
| return nil, fmt.Errorf("failed to determine current user for --my filter: empty user ID") | |
| } | |
| if cnf.API.EnableOrganizations { | |
| result = slices.DeleteFunc(result, func(p *api.ProjectInfo) bool { | |
| if p.OrganizationRef != nil { | |
| return p.OrganizationRef.OwnerID != myUserID | |
| } | |
| return true | |
| }) | |
| } |
| +--------+-----------+----------+ | ||
| | ID | Title | Region | | ||
| +--------+-----------+----------+ | ||
| | proj-1 | Project 1 | region-1 | | ||
| | proj-2 | Project 2 | region-2 | | ||
| +--------+-----------+----------+`) |
There was a problem hiding this comment.
The expected table output includes an extra leading "|" on the header and data rows (e.g., "|| ID..."), but RenderTable prints a single leading pipe ("| ID..."). This makes the test fail; update the expected string to match the actual table row format and spacing.
| +--------+-----------+----------+ | |
| | ID | Title | Region | | |
| +--------+-----------+----------+ | |
| | proj-1 | Project 1 | region-1 | | |
| | proj-2 | Project 2 | region-2 | | |
| +--------+-----------+----------+`) | |
| | ID | Title | Region | | |
| | proj-1 | Project 1 | region-1 | | |
| | proj-2 | Project 2 | region-2 |`) |
| fmt.Fprintf(cmd.OutOrStdout(), "No projects found (filters in use: %s).\n", strings.Join(filtersInUse, ", ")) | ||
| } else { | ||
| fmt.Fprintln(cmd.OutOrStdout(), "No projects found.") |
There was a problem hiding this comment.
When no projects are found, messages are written to stdout. In the legacy implementation these informational messages are written to stderr (stdout is reserved for machine-readable output like --pipe/--format). To preserve compatibility with scripts and existing expectations, write the "No projects found..." messages to cmd.ErrOrStderr() instead.
| fmt.Fprintf(cmd.OutOrStdout(), "No projects found (filters in use: %s).\n", strings.Join(filtersInUse, ", ")) | |
| } else { | |
| fmt.Fprintln(cmd.OutOrStdout(), "No projects found.") | |
| fmt.Fprintf(cmd.ErrOrStderr(), "No projects found (filters in use: %s).\n", strings.Join(filtersInUse, ", ")) | |
| } else { | |
| fmt.Fprintln(cmd.ErrOrStderr(), "No projects found.") |
| func (c *Client) GetMyProjects(ctx context.Context) ([]*ProjectInfo, error) { | ||
| // Get the current user's ID | ||
| meURL, err := c.resolveURL("users/me") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| var me struct { | ||
| ID string `json:"id"` | ||
| } | ||
| if err := c.getResource(ctx, meURL.String(), &me); err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // Get user extended access (project grants) | ||
| accessURL, err := c.resolveURL("users/" + url.PathEscape(me.ID) + "/extended-access?filter[resource_type]=project") | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| req, err := http.NewRequestWithContext(ctx, http.MethodGet, accessURL.String(), http.NoBody) | ||
| if err != nil { | ||
| return nil, Error{Original: err, URL: accessURL.String()} | ||
| } | ||
|
|
||
| resp, err := c.HTTPClient.Do(req) | ||
| if err != nil { | ||
| return nil, Error{Original: err, URL: accessURL.String(), Response: resp} | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| if resp.StatusCode != http.StatusOK { | ||
| return nil, Error{Response: resp, URL: accessURL.String()} | ||
| } | ||
|
|
||
| var accessResp userExtendedAccessResponse | ||
| if err := json.NewDecoder(resp.Body).Decode(&accessResp); err != nil { | ||
| return nil, Error{Original: err, URL: accessURL.String()} | ||
| } |
There was a problem hiding this comment.
GetMyProjects re-implements fetching the current user (users/me) and does a manual HTTP request/JSON decode for extended-access. This duplicates GetMyUserID and getResource logic and makes error/status handling inconsistent across the api package. Consider calling GetMyUserID and using c.getResource to fetch/decode userExtendedAccessResponse for consistency and less duplication.
| } | ||
|
|
||
| cmd.Flags().String("format", "table", "The output format: table, plain, csv, or tsv") | ||
| cmd.Flags().String("columns", "", "Columns to display (comma-separated)") |
There was a problem hiding this comment.
The legacy CLI's --columns option supports multiple occurrences and special behaviors like "+" (default columns placeholder) and wildcards (see legacy/src/Service/Table.php:66-73). This Go implementation only accepts a single comma-separated string and silently ignores unknown columns, which is a compatibility break for existing usage. Consider switching to a string slice flag and implementing the "+"/wildcard expansion + validation to match legacy behavior.
| cmd.Flags().String("columns", "", "Columns to display (comma-separated)") | |
| cmd.Flags().StringSlice("columns", nil, "Columns to display (comma-separated or repeated)") |
| // Check if no projects found and display appropriate message | ||
| if len(projects) == 0 { | ||
| filtersInUse := getFiltersInUse(cmd, cnf) | ||
| if len(filtersInUse) > 0 { | ||
| fmt.Fprintf(cmd.OutOrStdout(), "No projects found (filters in use: %s).\n", strings.Join(filtersInUse, ", ")) | ||
| } else { | ||
| fmt.Fprintln(cmd.OutOrStdout(), "No projects found.") | ||
| } |
There was a problem hiding this comment.
The "no projects" messaging differs from the legacy command (which prints "You do not have any projects yet." and shows follow-up guidance, and prints a different message when filters are in use). If the goal is exact behavior parity, align these messages and where they are written (stderr vs stdout) with legacy/src/Command/Project/ProjectListCommand.php:124-146.
|
Closing this one as it never materialized. We can open it again when we are ready to pursue this. |
Introduce a specialized agent to handle such migrations and migrate the
project:listcommand.Also introduce: