Refactor Repo Mirror List Sorting and Filtering

Claude Code·Opus 4.8[1m]·

gtrrz-victor·2mo ago·1hr 16min·5 Checkpoints·8 file changes·+330/-152·106.7K tokens

After reviewing this PR I've got the following feedback:

Inconsistent sortSpec handling between the two list paths.

The --show-available branch passes the raw sortSpec to sortRows (repo_mirror.go:446), while the main mirror branch passes the trimmed normalizedSort. Since sortRows computes 'desc := strings.HasPrefix(spec, "-")' on the raw spec, a leading-whitespace spec diverges: --show-available --sort " -repo" errors as an unknown column, while --sort " -repo" on the main list sorts descending. Unlikely input, but trivial to align by trimming once at the top of sortRows (or before both call sites).

Non-repo column sorts reintroduce the nondeterministic tie ordering this PR set out to fix.

Only the default (no --sort) and --sort repo/-repo get the (owner/repo, clusterHost) tiebreak via sortMirrorsDefault. Any other column (--sort private, --sort "clone url") falls through to sortRows -- a single-key stable sort with no secondary key -- so ties stay in whatever order the server/pagination returned. The code's own comment calls that order 'arbitrary', so e.g. entire repo mirror list --sort private over an all-private list can emit different orderings across runs.

Suggest threading the owner/repo secondary key into sortRows, or documenting that only the default / --sort repo ordering is fully deterministic. Related: no command-level test covers this sortRows branch for existing mirrors -- only the sortMirrorsDefault branch (--sort repo/-repo) is exercised end-to-end. After checking the sort code, it is hard to believe that we need that amount of code for doing something that trivial as sorting an array of elements, by just 3 different columns which are already known and not generic. I would rather having less generic methods.

2mo ago·33.6s

Done. All green.

What changed — `cmd/entire/cli/repo_mirror.go`

Deleted generic sortRows[T any] + separate sortMirrorsDefault. Replaced with concrete, non-generic sort per known column set:

parseSortColumn(spec, columns) — shared parser. Trims first, then reads - prefix. Direction and column name can no longer disagree. Both call sites route through it → whitespace handled identically.
sortMirrors — 3 known columns (repo/clone url/private). Always tiebreaks (owner/repo, clusterHost). repo/default = tiebreak alone (folds in old sortMirrorsDefault). - negates whole chain (kills the manual slice-reverse hack).
sortAvailable — 3 known columns (repo/access/status). Tiebreak owner/repo (no ClusterHost on AvailableMirror).
mirrorPrivate helper — shared by table row + sort key (DRY, killed goconst on "yes").

Call sites now one-liners: sortAvailable(avail, sortSpec) / sortMirrors(mirrors, sortSpec).

Fixes

Trim inconsistency — gone. Both paths pass raw spec; parseSortColumn trims before - check.
Nondeterministic ties — gone. Every column gets owner/repo(+cluster) secondary key.

Tests — `repo_mirror_test.go`

TestSortRows → replaced by TestSortMirrors + TestSortAvailable (default, -repo, non-repo tiebreak, whitespace " -repo", unknown column).
TestRepoMirrorList_FilterSort +2 end-to-end subtests: --sort private deterministic (owner/repo, cluster) order, --sort " -repo" whitespace parity. Existing subtests unchanged, pass.

Verify

go test ./cmd/entire/cli/ → ok (28.6s)
golangci-lint 2.11.3 → 0 issues
mise run fmt → clean

Note: local mise run lint errors on a shadowing go-installed golangci-lint v1.64.8 (PATH shadows the pinned 2.11.3). Env issue, not the code — ran the pinned binary directly. CI uses the action, unaffected.

One behavior note worth your call: two concrete sorters share a ~5-line comparator-chain shape. Kept separate to honor "less generic" — the genuinely shared bit (parseSortColumn) is factored once. Flag if you'd rather collapse.

Base directory for this skill: /Users/gtrrz-victor/.claude/skills/ship

Ship Changes

Commit current changes, push to remote, and update the PR.

Step 1: Analyze changes

Run git diff --stat and git diff to understand what changed
Run git status to see untracked files
Run git log --oneline -3 to match the repo's commit message style

Step 2: Commit

Stage only the relevant changed/untracked files by name (never git add -A or git add .)
Write a concise commit message that focuses on the "why" not the "what"
- If `` is provided, use it as the commit message
- Otherwise, draft one from the diff
Commit. If hooks fail, diagnose and fix — do NOT use --no-verify

Step 3: Push

Check if the branch tracks a remote: git rev-parse --abbrev-ref --symbolic-full-name @{u}
If not tracking, push with -u origin <branch>
If tracking, just git push
If push is rejected (remote has new commits), pull with rebase first then push

Step 4: Update PR

Find the open PR for the current branch: gh pr view --json number,body,title
If no PR exists, skip this step and inform the user
Compare the current PR body against the new changes:
- If the changes are within the existing PR scope, update the description to reflect the current state
- If the changes are additive or a follow-up, add a PR comment summarizing what was just pushed instead
When updating the PR body, preserve the overall structure but refresh the summary bullets and test plan
When adding a comment, keep it concise — bullet points of what changed and why
Before making any PR update (description edit or comment), show the user the exact text you plan to post and ask for their approval. Do NOT post until they confirm.

Rules

Never commit files that look like secrets (.env, credentials, tokens)
Never force push
Always show the user what was committed and the PR URL when done

2mo ago·1m

PR #1665 exists. This commit is a review-feedback follow-up within the PR's scope, but it makes the body's "Why" line (Shared sortRows/... helpers) stale. I'll post a comment rather than churn the auto-managed body trailers. Proposed comment:

Follow-up: addressed sort review feedback

Replaced the generic sortRows[T] / sortMirrorsDefault split with concrete sorters. Fixes two ordering bugs the review flagged:

Whitespace parity — desc was read from the raw spec but the column name from the trimmed spec, and the two list paths passed raw vs trimmed. Now a shared parseSortColumn trims before reading direction, so --sort " -repo" behaves the same on both the mirror and --show-available lists.
Deterministic ties on every column — only default / --sort repo had the (owner/repo, clusterHost) tiebreak; other columns fell back to a single-key sort with arbitrary server order. sortMirrors/sortAvailable now always apply the owner/repo(+cluster) tiebreak.

Also de-generalized per review — two concrete 3-column sorters instead of sortRows[T any]. Added unit tests (TestSortMirrors, TestSortAvailable) plus command-level coverage for --sort private tiebreak and --sort " -repo".

(Supersedes the body's "Shared sortRows helpers" note — sortRows/sortMirrorsDefault are gone.)

Post this comment?

Clean, tests pass. Column names now live in one place — the col* constants; both header arrays and both sort switches reference them.

Single source of truth:

The trick that made it work: parseSortColumn now returns the canonical header it matched (h) instead of a lowercased copy, so the sort switches match the uppercase constants directly (case colCloneURL:) — no strings.ToLower(const) awkwardness. Behavior identical: EqualFold still accepts any case from the user, the unknown-column error still prints lowercased names.

Want me to ship this follow-up?

No. Fixed.

Every column now runs through the same comparator chain with the tiebreak, not just repo:

--sort private → ties broken by owner/repo then cluster.
--sort "clone url" → same.
--sort access/status (available) → ties broken by owner/repo.

Old bug was non-repo columns bypassing sortMirrorsDefault into a single-key sort. That branch is gone — one code path, deterministic on every column.

Covered by tests: TestSortMirrors "non-repo column sorts keep the owner/repo+cluster tiebreak" (unit) + TestRepoMirrorList_FilterSort "--sort private breaks ties deterministically" (end-to-end). Both green.

what about this? Fragile ordering assertions: require.Less(strings.Index(stdout, a), strings.Index(stdout, b)).

strings.Index returns -1 when a substring is absent, and -1 < anyPresentIndex is true, so the assertion PASSES when the earlier (should-be-first) substring is missing entirely. Not currently exploitable -- these subtests apply no --repo filter and sorting never drops rows, so both operands are always present -- but it is safe only by accident of the fixtures. A future change pairing an ordering check with a filter, or any bug that drops the earlier row, would pass silently instead of failing.

Fix: assert require.Contains (or both indices >= 0) before comparing. Sites: ~525-526, 532-533, 564-567, 586-587, 592-595.

2mo ago·2m

Both are inside the requireOrder helper (one comment, one the actual call). Every assertion site converted.

Was it an issue yet? No — not currently exploitable, exactly as the review said: those subtests apply no --repo filter and sorting never drops rows, so both operands are always present. It was safe only by accident of the fixtures. Now it fails loudly regardless: requireOrder guards index >= 0 before comparing, so a dropped row or a filter+ordering combo can't pass silently.

fmt clean, lint 0 issues, all 10 subtests pass. Ship?

what about this one? Test "--repo/--sort apply under --show-available" never actually exercises --sort.

The fixture has two available rows (web/write, cli/read), but --repo cli filters down to a single row BEFORE --sort access runs, so the sort cannot reorder anything and is never observed. If the sortRows(avail, availableMirrorColumns, availableMirrorRow, sortSpec) call in the show-available branch were deleted or mis-wired (wrong column set), this test would still pass green.

Fix: keep >=2 rows after the filter (drop --repo, or use a substring matching both rows) and assert that 'read' orders before 'write'.

2mo ago·1m

Shipped.

Commit c1dfc2780 — repo mirror list: harden --sort test assertions
Pushed → feat/mirror-list-filter-sort

Both review points fixed:

Fragile ordering — new requireOrder helper guards index >= 0 before comparing; all sites routed through it. Absent/dropped rows now fail loudly instead of passing on -1 < present.
Sort never exercised under --show-available — fixture keeps 2 rows post-filter (delivered write-first), so --sort access must reorder to read-first; a deleted/mis-wired sortAvailable now fails the test.

No. Single sort key only.

Flag is StringVar(&sortSpec, "sort", ...) — one string. Passing --sort a --sort b just takes the last value (cobra overwrites), not both.
parseSortColumn parses one column + optional - prefix. No comma-splitting, no multi-key.

The tiebreak is fixed and implicit (owner/repo, then clusterHost) — not user-selectable. So you get one chosen column plus the deterministic fallback, never two user-specified sort columns like --sort private,repo.

Want me to add multi-column support (e.g. --sort private,-repo)?

Partially — and only by accident.

Comma-separated (--sort "private,repo"): errors, but as unknown sort column "private,repo"; valid columns: repo, clone url, private — because the whole string fails to match any column. Right outcome, misleading message (it's not really an unknown column, it's a rejected multi-key).
Repeated flag (--sort private --sort repo): no error — cobra's StringVar keeps the last value, so it silently sorts by repo. Silent, not rejected.

So there's no explicit "one column only" error. If you want the comma case to say something clearer (e.g. "only one sort column is supported"), I can special-case a comma in parseSortColumn. The repeated-flag case can't cleanly error without custom flag handling — cobra collapses it before we see it. Want either?

No clean built-in for a scalar flag. pflag doesn't count occurrences.

Flag.Changed is a bool ("was it set at all"), not a count — useless for detecting repeats.
For a scalar StringVar, pflag calls Set() once per occurrence and just overwrites; the earlier values are gone before you can see them. Nothing records that it happened twice.

So the only ways to detect multiple --sort:

A slice type (StringArrayVar) — count len(). What we did. Idiomatic, one line.
A custom pflag.Value whose Set() increments a counter — lets the stored value stay a plain string but you own the multi-set detection:

That's strictly more code than StringArrayVar for the same result — you're hand-rolling what the slice type gives free.

Manually scan os.Args — hacky, don't.

My recommendation: keep StringArrayVar. It is the cobra-idiomatic way to catch repeated flags; there's no lighter switch. Want me to leave it as-is (tests were passing) or would you prefer the custom Value?

is it a issue yet? Passing --sort repo explicitly routes to sortRows, which sorts only on the REPO cell and does not break ties by ClusterHost, whereas the default (no --sort) sortMirrorsDefault does. A repo mirrored on multiple clusters is therefore ordered deterministically by default but in arbitrary server order under --sort repo, contradicting the documented equivalence and the determinism guarantee. Consider applying the same cluster tie-break for the repo column (or documenting the divergence).

2mo ago·19.1s

No — already fixed. That review describes the old sortRows/sortMirrorsDefault code, both of which are gone.

Now --sort repo and default both go through the same sortMirrors. For the repo column the primary key returns "", so every row falls straight into the shared tiebreak:

So --sort repo gets the exact (owner/repo, clusterHost) ordering the default does — the divergence is structurally impossible now; there's one code path.

Guarded by the regression test explicit --sort repo keeps the cluster tiebreak (matches default) — asserts aws-cluster before eu-cluster within a duplicate-repo tie under --sort repo. Green.

Fixed. --repo now matches the displayed owner/repo string on both paths.

--repo acme/web (copied from the REPO column) → matches the row. Was returning nothing.
--repo cli still works (substring of acme/cli).
Comment + flag help updated; regression test --repo matches the owner/repo form shown in the REPO column added.

fmt clean, lint 0 issues, all mirror tests pass.

One behavior note for your call: matching owner/repo means --repo now also matches the owner substring — e.g. --repo acme matches every acme/* repo (previously it only matched repos literally named acme). That's consistent with the displayed column, and --owner remains the server-side owner filter. Flag if you'd rather keep --repo repo-name-only and instead match either bare repo or owner/repo (so both web and acme/web work without broadening to owner).

Ship this?