Skip to content

fix(validator): tokenize gh-list-no-limit's flag check instead of substring match - #1147

Merged
potiuk merged 1 commit into
apache:mainfrom
AmirF194:fix/gh-list-limit-comment-bypass
Sep 7, 2026
Merged

fix(validator): tokenize gh-list-no-limit's flag check instead of substring match#1147
potiuk merged 1 commit into
apache:mainfrom
AmirF194:fix/gh-list-limit-comment-bypass

Conversation

@AmirF194

@AmirF194 AmirF194 commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • validate_gh_list_limit decides whether a gh issue list / gh pr list call is missing
    --limit by testing whether the literal substring --limit occurs anywhere on the
    logical line, so a trailing shell comment or a quoted argument that merely mentions
    --limit silently suppresses the warning even when the command has no such flag.
  • Fixes it by tokenizing the line with shlex.split(..., comments=True) and checking for an
    actual --limit (or --limit=N) token instead of a bare substring match.

Type of change

  • Python package (tools/*/ with pyproject.toml)

Test plan

  • prek run --all-files passes
  • For Python packages touched: uv run pytest / ruff check / mypy passes
  • Added three cases to TestGhListLimit: a trailing comment containing --limit, a quoted
    argument containing --limit, and the --limit=100 equals-form (control, must stay
    silent). The first two fail on main and pass on this branch, verified both ways on
    Python 3.12. Full tests/test_validator.py (466 cases) still passes.
  • I have not checked every --limit spelling gh itself accepts, only --limit N and
    --limit=N.

RFC-AI-0004 compliance

  • Sandbox: no new host access, pure text processing like the code it replaces

Linked issues

Self-discovered while reading the validator surface; no existing issue.

Notes for reviewers (optional)

_GH_LIST_RE matching and the fenced-block boundary are unchanged. The only touched
function is validate_gh_list_limit, single call site.

…string match

validate_gh_list_limit tested for the literal substring "--limit" anywhere on
the logical line, so a trailing shell comment or a quoted argument that
merely mentions "--limit" silently defeated the check even when the command
had no such flag. Tokenize with shlex.split(comments=True) and match an
actual --limit / --limit=N token instead.

Fixes apache#1146

Generated-by: Claude Code (Sonnet 5)
@potiuk
potiuk force-pushed the fix/gh-list-limit-comment-bypass branch from 5d3b0ce to febf968 Compare September 7, 2026 22:51

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good fix, and the right shape: a substring test on a shell line was always going to over-match, and tokenising is the correct answer rather than layering more string heuristics on top.

I verified the behaviour on your branch rather than just reading it:

Input Flagged
--limit in a trailing comment yes (was wrongly silent)
--limit inside --search "..." yes (was wrongly silent)
real --limit 100 no
--limit=100 no
unbalanced quote yes

The except ValueError: tokens = [] path is the detail I liked most — an unparseable line flags rather than silently passing, and the comment says why. That is the correct direction to fail for a lint rule, and it is easy to get backwards.

One non-blocking nit inline about the -L short form. I am resolving it so it does not block the merge; it can be a follow-up.


🤖 This review was drafted by an AI-assisted tool and may contain mistakes. It has been reviewed and confirmed by an Apache Magpie maintainer before submission. See CONTRIBUTING.md for what this project considers a maintainer review.

@potiuk
potiuk merged commit 7dfb956 into apache:main Sep 7, 2026
10 checks passed
@AmirF194

AmirF194 commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Appreciate you actually running the edge cases instead of just reading the diff, the trailing-comment and quoted-argument rows were the ones I was least sure about. Good call resolving the -L nit as a follow-up rather than blocking on it.

@AmirF194
AmirF194 deleted the fix/gh-list-limit-comment-bypass branch September 7, 2026 23:15
potiuk added a commit that referenced this pull request Sep 8, 2026
The marker sat at a1cff44, 17 commits behind main. Bumping it alone
would claim those commits are described by the specs, so the drift is
closed first.

Specs updated for what actually shipped:

- meta-and-quality-tooling: skill-evals errors, rather than passing, when
  a case's CLI produced no gradeable output (#1161).
- security-reporting: the tracker dashboard projects the current partial
  bucket to its end-of-bucket value, splitting RATE series (accumulate
  from zero) from LEVEL series (carry over), and deliberately not
  projecting mean-based signals (#1158).
- project-agnosticism: <PROJECT> and <project> are two placeholders
  holding different values, and the lint carries both spellings plus
  spaced variants (#1154).
- adapters: the forwarder relay's contact_handle defaults to an
  org-level shared inbox rather than a named individual (#1135). The
  multi-hop coordinator case is designed in RFC-AI-0008 and unimplemented.
- issue-management-family: the family's eval suites, and the note that
  --cli runs belong outside a credential-denying sandbox (#1145).

Commits needing no spec change: #1152, #1143 and #1156 updated their own
specs in-commit; #1149, #1147, #1151 are behaviour-preserving bug fixes;
#1155 and #1141 are CI and dependency chores; #1159's spec edits landed
with it; #1144 removes hardcoded literals that no spec asserted.

One genuine gap recorded rather than papered over: no spec covers
marketplace distribution or the dev-version stamping rule from #1160,
which is load-bearing because `claude plugin update` compares version
strings, not commit SHAs. Logged in adoption-and-setup as wanting its own
spec.

Generated-by: Claude Code (Opus 5)
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