Skip to content

fix(code-review): make the documented "." catch-all actually match - #1

Merged
starknetdev merged 1 commit into
mainfrom
fix/catch-all-diff-paths
Jul 29, 2026
Merged

fix(code-review): make the documented "." catch-all actually match#1
starknetdev merged 1 commit into
mainfrom
fix/catch-all-diff-paths

Conversation

@starknetdev

@starknetdev starknetdev commented Jul 29, 2026

Copy link
Copy Markdown
Member

Summary

The README's catch-all example does not work. diff_paths are matched with startswith(), so "." matches only paths that literally begin with a dot:

$ jq --argjson changed_files '["README.md","Scarb.toml"]' -f matcher.jq cfg.json
{"include":[]}          # diff_paths: ["."], exclude_paths: ["packages/"]

A repo following the documented pattern gets no review at all on a PR that touches only README.md, package.json, Scarb.toml, codecov.yml — anything not under a dotted path. It fails silently: no error, no skipped job, the agent simply never enters the matrix.

It is also easy to miss in review, because the pathspec half of the same value is correct. git diff -- . ':!packages/' scopes exactly right, so on the runs where the agent does trigger (a .github/** change, say) it reviews the right files. Only the trigger decision is wrong.

Found while porting game-components to this workflow (Provable-Games/game-components#122), which had to enumerate every top-level path as a workaround.

The fix

  • Normalize "." / "./" / "" to the empty prefix in the matcher. Every path starts with "", so a catch-all matches everything, and exclude_paths still carves out the app dirs.
  • Map "" back to "." when building diff_paths_str, since "" is not a valid git pathspec but "." is and means the same thing. So [""] and ["."] now behave identically on both halves.
  • Document that these are literal prefixes, not globs. "src/**" matches nothing today and gives no warning — worth stating outright.

Verification

No behavior change for existing callers. I extracted the matcher from the workflow before and after, and ran both against the two live configs — denshokan-sdk and game-components — across 10 changed-file sets each:

differences: 0

The catch-all now works. With diff_paths: ["."], exclude_paths: ["packages/"]:

changed files agents matched
README.md general
Scarb.toml general
codecov.yml general
.github/workflows/x.yml general
packages/a/b.cairo packages
packages/a/b.cairo + README.md packages, general

Emitted pathspec is valid for both spellings: git diff -- . :!packages/.

Edge cases: an empty agents array returns {"include":[]} rather than erroring, and "src/**" still matches nothing — now called out in the README instead of silently surprising the next person.

npx yaml@2.8.1 valid passes on the workflow.

Consumer impact (all 6 live callers audited)

gh search code 'code-review.yml@v1' --owner Provable-Games finds six consumers. I ran the pre- and post-patch matchers against every one of their real configs across 12 changed-file sets:

repo uses a "." catch-all result
denshokan-sdk no 12/12 identical
budokan-sdk no 12/12 identical
on-ramp-sdk no 12/12 identical
metagame-sdk no 12/12 identical
budokan yes general agent starts matching
metagame_extensions yes general agent starts matching

Only entries that are exactly ".", "./" or "" change meaning, so a config without one is bit-identical by construction.

budokan and metagame_extensions are the two repos the bug is currently biting. Their general agent almost never runs today — a README.md, package.json, docs/** or LICENSE PR matches nothing at all:

budokan              ["README.md"]                     [] -> [general]
budokan              ["contracts/a.cairo","README.md"] [contracts] -> [contracts,general]
metagame_extensions  ["docs/x.md"]                     [] -> [general]

That is the fix landing, not a regression — those PRs are going unreviewed right now. But it is a live behavior change on two active repos, so it should be a conscious merge rather than a surprise.

It cannot turn a green build red. The only exit 1 paths in this workflow are a missing config file and a non-zero exit from the codex CLI itself; findings are never gated on severity. Both repos call this from a standalone pr-review.yml with no aggregate job depending on review, so nothing gates a merge on it either. The blast radius is more review jobs, more PR comments, and more spend on those two repos.

After merge

This is backward-compatible (0 diffs above), so per the README's versioning note the v1 tag should move forward to pick it up — callers pin @v1. I have not moved the tag.

Once it does, game-components can drop its enumerated path list back to the documented two-line catch-all.

🤖 Generated with Claude Code

diff_paths are matched with startswith(), so the README's catch-all
example — diff_paths ["."] plus exclude_paths — only ever matched
dotfile paths. A repo whose PR touched just README.md, package.json or
Scarb.toml got no review at all, silently.

The failure is easy to miss because the pathspec half of the same value
is correct: `git diff -- . ':!packages/'` scopes exactly right, so the
agent reviews the right files on the runs where it does trigger.

Normalizes "." / "./" / "" to the empty prefix in the matcher (every
path startswith ""), and maps "" back to "." when building the git
pathspec, since "" is not a valid pathspec. Also documents that these
are literal prefixes, not globs — "src/**" matches nothing today and
gives no warning.

Verified no behavior change for existing callers: the patched and
original matchers produce identical output for denshokan-sdk's and
game-components' configs across 10 changed-file sets each.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@gemini-code-assist

Copy link
Copy Markdown

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@starknetdev
starknetdev merged commit ce28528 into main Jul 29, 2026
starknetdev added a commit to Provable-Games/game-components that referenced this pull request Jul 29, 2026
Both depend on shared-workflow changes that have now landed at v1.

review-agents.json: the general agent goes back to the documented
diff_paths ["."] + exclude_paths ["packages/"]. The enumerated top-level
list was a workaround for the prefix matcher treating "." as a dotfile
prefix (fixed in Provable-Games/.github#1), and it silently missed any
new root file — LICENSE and docs/** matched no agent at all.

pr-ci.yml: block_on_severity: true restores the merge gate the inline
review jobs had (Provable-Games/.github#2). Without it a [CRITICAL]
finding would only post a comment.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
starknetdev added a commit to Provable-Games/game-components that referenced this pull request Jul 29, 2026
* chore(ci): delegate AI review to the org-shared reusable workflow

Replaces four hand-rolled review jobs (claude/codex x packages/general)
with a single call to Provable-Games/.github code-review.yml@v1. Review
logic, prompt assembly, comment upserting and model config now live in
one place, so a model deprecation is an org-variable change rather than
an edit here. This repo supplies only .github/review-agents.json plus
the existing .github/prompts/*.md.

Net -577 lines. Also drops the local AI-availability gate: the shared
workflow owns fork skipping, so the `changes` job no longer computes
can_run_ai_reviews and the pr-ci aggregate is back to a plain
did-any-job-fail check.

The general agent enumerates top-level paths rather than using the
documented ["."] catch-all: the shared workflow's matcher is prefix
based, so ["."] only matches dotfiles and would silently skip review on
README.md / Scarb.toml / codecov.yml-only PRs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* chore(ci): use the documented catch-all and restore the severity gate

Both depend on shared-workflow changes that have now landed at v1.

review-agents.json: the general agent goes back to the documented
diff_paths ["."] + exclude_paths ["packages/"]. The enumerated top-level
list was a workaround for the prefix matcher treating "." as a dotfile
prefix (fixed in Provable-Games/.github#1), and it silently missed any
new root file — LICENSE and docs/** matched no agent at all.

pr-ci.yml: block_on_severity: true restores the merge gate the inline
review jobs had (Provable-Games/.github#2). Without it a [CRITICAL]
finding would only post a comment.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* docs(ci): record why the review job pins @v1 and inherits secrets

Both were flagged as reproducibility/least-privilege risks in review.
Keeping them, deliberately: @v1 is the org convention every caller uses
and is what makes an upstream model change a one-place fix, and the
callee is first-party and declares exactly the two secrets it uses.
Pinning a SHA or enumerating secrets here would diverge from the other
six callers for no practical gain.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

1 participant