Skip to content

flag unbounded gh issue/pr list calls (no --limit)#216

Merged
potiuk merged 1 commit into
apache:mainfrom
justinmclean:gh-list-length
May 25, 2026
Merged

flag unbounded gh issue/pr list calls (no --limit)#216
potiuk merged 1 commit into
apache:mainfrom
justinmclean:gh-list-length

Conversation

@justinmclean

Copy link
Copy Markdown
Member

What

Adds a new validate_gh_list_limit check that flags gh issue list and
gh pr list calls in fenced code blocks that have no --limit argument.

Why

gh issue list and gh pr list default to 30 results. On a large repo
like apache/airflow with thousands of open issues, this silently truncates
— any downstream logic (counting, filtering, looping) operates on an
incomplete set with no error or warning. Adding --limit makes the intent
explicit and prevents hard-to-spot data loss.

Changes

tools/skill-validator

  • New GH_LIST_CATEGORY constant (gh_list_no_limit), added to SOFT_CATEGORIES
  • New _join_continuations helper: joins shell line-continuations (\ + newline)
    before checking, so --limit on a continuation line is correctly recognised
  • New validate_gh_list_limit — scans fenced blocks only; prose and inline
    backtick mentions are not flagged
  • Wired into run_validation for all skill .md files
  • "gh-list-no-limit" added to _SOFT_RULE_PREFIXES
  • 9 new tests in TestGhListLimit

Real gaps found (3)

  • pr-management-triage/actions.md:517
  • pr-management-code-review/selectors.md:119
  • security-issue-invalidate/SKILL.md:246

The selectors.md:484 call (gh pr list \ ... --limit 100 across continuation
lines) is correctly silent — the multiline join handles it.

Notes

All violations are SOFT — advisory warnings that do not fail CI unless
--strict is passed. Even so, the 3 flagged files have been fixed.

@potiuk

potiuk commented May 18, 2026

Copy link
Copy Markdown
Member

Hmm. Should we allow configurable number ? And possibly at least a flag or notification telling "and there is more" ?

@justinmclean

Copy link
Copy Markdown
Member Author

Good points. On the configurable number: the validator only checks for the presence of --limit, not the value, so any number works. Just edit the snippet directly. I decided a dedicated abstraction (env var, shared config) is not worth the overhead for now.

On "and there is more": added a prose note to the three affected skill files instructing Claude to note in its response if the result count equals the --limit value, so the user knows there may be additional results not shown.

@potiuk

potiuk commented May 19, 2026

Copy link
Copy Markdown
Member

Good points. On the configurable number: the validator only checks for the presence of --limit, not the value, so any number works. Just edit the snippet directly. I decided a dedicated abstraction (env var, shared config) is not worth the overhead for now.

Indeed - LLM should figure on its own and allow override the default in overrides.

On "and there is more": added a prose note to the three affected skill files instructing Claude to note in its response if the result count equals the --limit value, so the user knows there may be additional results not shown.

yep. Good enough. Let LLM propos what to do. It will likely figure out when such issue happen and will propose modifications.

@potiuk

potiuk commented May 20, 2026

Copy link
Copy Markdown
Member

Hi @justinmclean — heads-up: this PR currently shows as conflicting against main.

The conflicts are in tools/skill-validator/src/skill_validator/__init__.py and tools/skill-validator/tests/test_validator.py and overlap with merged #220 ("detect Pattern 4 injection-guard callout"), which reorganised the validator registry and the test suite in the same area.

I had a look at rebasing it on your behalf, but resolving how the new validator from this PR should coexist with the Pattern 4 infrastructure feels like a judgement call you should make rather than me guessing. Could you rebase against latest main and resolve when you get a moment? Happy to re-review once it's clean.

Thanks!

@justinmclean justinmclean self-assigned this May 20, 2026
Generated-by: Codex (GPT-5)
@potiuk potiuk merged commit a171e60 into apache:main May 25, 2026
13 checks passed
@justinmclean justinmclean deleted the gh-list-length branch May 28, 2026 00:29
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