Refactor Grant Revoke Routing and Tests
some issues on this branch still, from reviewers:
revokeProjectGrantee/revokeRepoGrantee are near-identical (~18 lines each) — confirm dupl (threshold 75) stays green; if it trips, a //nolint:dupl with rationale per CLAUDE.md is the expected fix. No test for granteeName or the new list columns. The granteeName(name, id) fallback (friendly name vs ULID) and the expanded grantColumns/projectGrantRow/repoGrantRow are entirely untested. It's trivial branching, but it's user-facing output and exactly the kind of thing a 3-line table test pins cheaply — especially the empty-OptString→ULID fallback. Worth adding before merge.
Both reviewer concerns addressed.
Dupl (concern 1): Confirmed green. mise run lint (threshold 75, CI-enforcing) reports nothing for grant.go — revokeProjectGrantee/revokeRepoGrantee don't trip at 75. No //nolint:dupl needed. (The advisory mise run dup at threshold 50 flags the list commands, not the revoke pair, and is advisory-only.)
Tests (concern 2): Added to grant_test.go:
TestGranteeName— friendly name wins; unsetOptString→ULID; empty-stringOptString→ULID fallback.TestGrantRows— pinsgrantColumnswidth (5) plus full row output forprojectGrantRow(resolved name) andrepoGrantRow(unresolved → ULID), checking column order/alignment.
All pass; lint clean. Nothing committed — yours to push.
can we dedupe the list or revoke commands at all? Or doesn't that make sense?
Done. Both functions now delegate to a shared revokeGrantee helper; the ULID-vs-handle routing and message formatting live in one place. Message output is byte-identical to before (account X from project Y / X from project Y).
- Lint clean, tests pass.
- List commands left as-is (deliberately — marginal gain, readability cost, not a CI blocker).
Nothing committed — yours to push.
does 'list' need deduping?
No — and I'd actively avoid it.
- It's not real logic duplication, it's three distinct API shapes. Each list differs in ref resolver (
resolveOrgRef/resolveProjectRef/resolveRepoRef), param type + ID field (OrgId/ProjectId/RepoId), list method, and result field (out.Membersvsout.Grants). Repo also carries--project. The only literally-identical part is the cursor/error boilerplate inside thefetchAllPagesclosure. - A generic helper costs more than it saves. To collapse it you'd thread 3–4 type params and 3–4 function args (
newParams,setPageToken,call,items,next) per call site. The call sites wouldn't get shorter — they'd get harder to read, swapping obvious code for clever code. - No maintenance win. If one endpoint's pagination changes, it changes independently regardless. There's no shared invariant to protect.
- No CI pressure. It only trips advisory
dup(threshold 50), not lint (75).
The revoke dedup was worth it because the route-decision logic (ULID vs handle + message formatting) was genuinely shared. List has no equivalent shared decision — just parallel boilerplate. Leave it.
okay, cool. commit and push.
Pushed. 4bf8bdf11 on paul/cor-699-cant-repo-grant-with-entire-cli.