Skip to content

fix(repo): stop misreading flag values as owner positional in repo list - #87

Open
ahuang100 wants to merge 3 commits into
kunchenguid:mainfrom
ahuang100:fix/repo-list-owner-positional-arg
Open

fix(repo): stop misreading flag values as owner positional in repo list#87
ahuang100 wants to merge 3 commits into
kunchenguid:mainfrom
ahuang100:fix/repo-list-owner-positional-arg

Conversation

@ahuang100

Copy link
Copy Markdown

What Changed

  • repo list no longer misreads a flag's value (e.g. --limit's number, or --visibility/--language values) as the owner positional argument, so gh repo list is invoked with the correct owner instead of a spurious one.
  • getFlag/takeFlag in src/args.ts now reject a value that itself looks like another flag (starts with --) instead of consuming it, closing a related case where a dangling single-value flag (e.g. --limit --visibility public) could swallow the next flag's token; the --flag=value form remains unaffected.
  • Documented the new dangling-flag rejection behavior for getFlag/takeFlag in AGENTS.md.

Risk Assessment

✅ Low: The new commit fixes the round-1 finding at the correct shared boundary (args.ts) rather than a local patch, is covered by targeted tests that reproduce the exact failing sequence, and its only collateral effect (free-text flag values starting with "--" now require the --flag=value form) has a working escape hatch and doesn't reintroduce the original bug.

Testing

Ran the targeted vitest suites for args.ts and the repo/run commands (98 tests, all passing, including new regression tests added in this change), then independently reproduced the original bug end-to-end against a real authenticated gh CLI session by running gh-axi repo list --limit 3 on the base commit (owner positional wrongly swallowed the limit value, producing the wrong repo list) versus the target commit (correct behavior, lists the authenticated user's own repos). Working tree is clean and the temporary comparison worktree was removed after use.

Evidence: Before/after CLI transcript for gh-axi repo list --limit bug
# `gh-axi repo list --limit N` owner-positional bug: before/after CLI transcript

Command run in both cases: `npx tsx bin/gh-axi.ts repo list --limit 3`
(authenticated `gh` account: ahuang100)

## Base commit fe384b3 (bug present)

`--limit`'s value `3` was mistaken for the owner positional, so the CLI ran
`gh repo list 3 --json ... --limit 3` instead of listing the authenticated
user's own repos. Output was wrong/unexpected repos:

`` `
count: 2
repos[2]{name,description,visibility,language,stars,updated}:
  dotfiles,"",public,Vim Script,0,12y ago
  pair-box,"",public,null,0,12y ago
help[1]:
  Run `gh-axi repo view --repo <owner/name>` to view a repository
`` `

## Target commit 101fa57 (fix applied)

`--limit`'s value is consumed by `takeFlag` before positionals are scanned,
so it can no longer be mistaken for the owner. Output correctly lists the
authenticated user's own repos, limited to 3:

`` `
count: 3 (showing first 3)
repos[3]{name,description,visibility,language,stars,updated}:
  gh-axi,GitHub CLI for agents — designed with AXI (Agent eXperience Interface).,public,TypeScript,0,1h ago
  AGL,"AGL - Adversarial Generative Loop ",private,Python,0,2mo ago
  helical_cot,Results for the helical CoT experiments,private,Jupyter Notebook,0,2mo ago
help[1]:
  Run `gh-axi repo view --repo <owner/name>` to view a repository
`` `
Evidence: Targeted vitest run (args.ts, repo.ts, run.ts)
test/args.test.ts (45 tests) passed
test/commands/repo.test.ts (23 tests) passed
test/commands/run.test.ts (30 tests) passed
All 98 tests passed

Pipeline

Updates from git push no-mistakes

⏭️ **intent** - skipped

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 2 infos
  • ⚠️ src/commands/repo.ts:173 - The fix correctly stops a flag's value from being misread as the owner positional in the common case, but the four sequential takeFlag/takeBoolFlag calls share the same mutable args array and are destructive. If a flag is given without its value directly followed by another not-yet-consumed flag (e.g. gh-axi repo list --limit --visibility public), takeFlag('--limit') swallows the literal token '--visibility' as limit's value and splices both out, leaving 'public' as a stray non-flag token that the owner-positional scan (positionals[1]) then picks up and inserts into the gh invocation as the owner (gh repo list public ...). In practice this still surfaces as a gh-level parse error (since '--visibility' becomes an invalid --limit value), so it is not a silent wrong-owner success, but it is a narrow reappearance of the same bug class (a non-owner token landing in the owner slot) for malformed input. Consider having takeFlag reject/ignore a 'value' that itself starts with '--' so a dangling flag can't consume an adjacent real flag.

🔧 Fix: args: reject flag-like values so dangling flags can't swallow the next flag
2 infos still open:

  • ℹ️ src/args.ts:12 - The dangling-flag fix from round 1 was correctly placed at the shared boundary (getFlag/takeFlag/requireFlagValue in args.ts) rather than patched only in repo.ts, so the invariant now holds everywhere args flow through these helpers, and the new tests (args.test.ts, repo.test.ts, run.test.ts) confirm the exact --limit --visibility public repro now throws cleanly instead of leaking a token into the owner positional. One foreseeable side effect worth being aware of: this also applies to free-text flags across the codebase (--title, --description, --comment, --subject, --milestone, --query, etc. in pr.ts/issue.ts/project.ts/label.ts), so a legitimate value that itself starts with "--" (e.g. --title &#34;--wip: fix login&#34;) is now rejected with a confusing "<flag> requires a value" message even though a value was supplied. The --flag=value equals-form still bypasses the check as an escape hatch (both getFlag and takeFlag only reject in the space-separated branch), so nothing is actually unrepresentable, but the escape hatch isn't documented anywhere.
  • ℹ️ CLAUDE.md - CLAUDE.md's "Repeatable flags" section documents that getAllFlags/takeAllFlags reject dangling/blank values, but doesn't yet mention that getFlag/takeFlag/requireFlagValue also now reject a value that looks like another flag (starts with "--") as of this change. Since this file's stated purpose is to capture exactly this kind of durable, cross-cutting sharp edge so it isn't rediscovered, a short follow-up note here (and that --flag=value is the escape hatch for a literal dash-prefixed value) would match the existing documentation bar.
✅ **Test** - passed

✅ No issues found.

  • npx vitest run test/args.test.ts test/commands/repo.test.ts test/commands/run.test.ts - 98 tests passed, including the new cases 'does not mistake the --limit value for the owner positional when no owner is given', 'passes an explicit owner positional through to gh repo list', 'does not mistake --visibility or --language values for the owner positional', and dangling/flag-like-value rejection tests in args.test.ts
  • Manual E2E: ran npx tsx bin/gh-axi.ts repo list --limit 3 against the live authenticated gh CLI (account ahuang100) on target commit 101fa57 - correctly listed the authenticated user's own repos (gh-axi, AGL, helical_cot)
  • Manual E2E regression repro: checked out base commit fe384b3 in a temporary git worktree and ran the identical command - reproduced the bug, where the limit value '3' was misread as the owner positional, producing wrong output (dotfiles, pair-box instead of the user's actual repos); temporary worktree removed afterward
✅ **Document** - passed

✅ No issues found.

✅ **Lint** - passed

✅ No issues found.

✅ **Push** - passed

✅ No issues found.

listRepos scanned args for positionals by only excluding tokens
starting with --, so a bare value belonging to --limit, --visibility,
or --language (e.g. the 10 in --limit 10) was mistaken for the
optional owner positional whenever no owner was given. This made
`gh-axi repo list --limit 10` silently query gh repo list 10 instead
of the authenticated user's repos.

Consume those value-taking flags with takeFlag/takeBoolFlag before
computing positionals, matching the existing convention elsewhere in
the codebase (pr.ts uses takeAllFlags for the same reason).
@ahuang100

Copy link
Copy Markdown
Author

This PR was raised to address a small issue I found when instructing claude sonnet to retrieve all my repos using gh-axi, only to have it return public repos from another user. This is my take on how flags should be consumed but the no-mistakes review loop did note some consequences based on this design.

misc.
Thanks to Kun and the community for the great work done in gh-axi as well as no-mistakes.

@kunchenguid

Copy link
Copy Markdown
Owner

Speaking as Kun's firstmate: I reviewed the actual diff (src/commands/repo.ts, src/args.ts, src/commands/run.ts, AGENTS.md, tests). Consuming --limit/--visibility/--language before the owner positional is a genuine corrective fix for gh-axi repo list --limit N querying the wrong owner. Checks are green, including no-mistakes.

I will not auto-merge. The PR is conflicting with main. Independently, getFlag/takeFlag now reject any space-separated value that starts with -- across the whole CLI, which is a default-behavior change (a real --title "--wip" value now requires --flag=value). Default-behavior changes are not auto-merged, and conflicts are not resolved unless a PR is otherwise auto-merge-ready.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants