Skip to content

Add Claude automated code review#56

Open
mattpodwysocki wants to merge 18 commits intomainfrom
add-claude-review
Open

Add Claude automated code review#56
mattpodwysocki wants to merge 18 commits intomainfrom
add-claude-review

Conversation

@mattpodwysocki
Copy link
Contributor

Summary

Adds .github/workflows/claude-review.yml using anthropics/claude-code-action@v1.

  • Auto-reviews PRs with ≥300 changed lines
  • Responds to @claude mentions in PR comments
  • Uses ANTHROPIC_API_KEY repo secret

Prompt focus areas

Tailored for a markdown skills repo:

  1. Accuracy — technical content correct for referenced Mapbox APIs/SDKs
  2. Skill structure — valid frontmatter schema per CONTRIBUTING.md
  3. Clarity — actionable guidance, complete runnable code examples
  4. Consistency — tone and depth match existing skills
  5. Scope — skills stay focused, no unnecessary overlap

Setup required

Add ANTHROPIC_API_KEY to repository secrets:
Settings → Secrets and variables → Actions → New repository secret

🤖 Generated with Claude Code

mattpodwysocki and others added 16 commits March 10, 2026 13:39
15-slide deck covering: what skills are, how to install them, skill
structure, all 18 skills by domain, deep dives on web performance /
iOS / Android / MCP integration, eval benchmarks, and contributing.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tors)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r file)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tegy

Four new slides before the existing 'What Are Agent Skills?' slide that
build the narrative from developer pain to the skills solution and
Mapbox's specific motivation for publishing them.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Covers: evals.json format, how to pick good discriminating prompts,
what makes a good expectation, the benchmark directory structure,
and how to interpret +0pp results.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two new slides after Contributing: one mapping the gaps (Flutter/React
Native, custom rendering, offline maps, testing, i18n, Tiling Service)
and one framing each gap as an opportunity with a concrete fork-and-PR
call to action for community contributors.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…cent equivalents

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Move tools/skills/prompts taxonomy to slide 3 (define before explaining)
- Rename fine-tuning/RAG comparison to 'Why Agent Skills?' (slide 4)
- Move 'Skills + MCP: Better Together' after deep dives (not after evals)
- Move CTA slides (gaps, opportunity) after contributing/evals section
- Result: Problem → Define → Why → Mapbox → Strategy → Use → Explore → Measure → Contribute → CTA

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Triggers on PRs with >=300 changed lines and on @claude mentions.
Uses ANTHROPIC_API_KEY repo secret.
Prompt tailored to skills repo: accuracy, skill schema structure,
clarity of guidance, and consistency with existing skills.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@mattpodwysocki mattpodwysocki requested a review from a team as a code owner March 13, 2026 00:07
mattpodwysocki and others added 2 commits March 13, 2026 15:31
The claude-code-action tries the GitHub App OIDC token exchange first
when id-token: write is present. Without the app installed it fails
before falling back to the ANTHROPIC_API_KEY secret.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The action uses OIDC to get an installation token from the Claude Code
GitHub App, but the app isn't installed. Passing github_token explicitly
makes the action use GITHUB_TOKEN for all GitHub API calls instead.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@github-actions
Copy link

github-actions bot commented Mar 13, 2026

Claude finished @mattpodwysocki's task in 1m 54s —— View job


Review of PR #56 — Add Claude automated code review + presentation deck

  • Gather context (read CLAUDE.md, CONTRIBUTING.md, existing skills)
  • Review workflow file (claude-review.yml)
  • Review presentation files
  • Review ignore file changes
  • Post review feedback

Summary

This PR adds two things: (1) a GitHub Actions workflow for Claude-powered code review, and (2) a Slidev presentation deck covering the Mapbox Agent Skills project. Overall the PR is well-structured and the presentation content is technically sound. A few issues worth addressing:

Workflow (claude-review.yml)

1. Unnecessary id-token: write permission — Since the workflow authenticates via ANTHROPIC_API_KEY (not GitHub OIDC), this permission should be removed for least-privilege. The commit history suggests this was intentionally removed at one point (fcb84e9) but reappeared. See inline comment →

2. Fragile line-count parsing — The git diff --stat | awk approach can miscount because gsub(/[^0-9]/, " ") doesn't distinguish file counts from insertion/deletion counts. git diff --numstat is more reliable. See inline comment →

3. Long claude_args line — The single-line string is ~430 characters. A YAML block scalar would improve readability. See inline comment →

Presentation (slides.md)

The presentation is well-organized, technically accurate, and provides good coverage of the skills ecosystem. A few minor notes:

4. Workflow prompt references non-existent triggers frontmatter field — The review prompt (line 48 of the workflow) says to check for "triggers, content" in skill frontmatter, but per CONTRIBUTING.md the schema is name + description only. Trigger conditions are embedded in the description text. The slide deck's own example (slide on Skill Structure) is correct, but the workflow prompt should be updated to match. See inline comment →

5. README slide list is out of sync — The README.md lists 15 slides in an order that doesn't match the actual deck (which has ~25 slides). Several slides like "Why Agent Skills?", "Why Mapbox Needs Skills", "Skills as a Strategy", "Skills + MCP: Better Together", and the eval-related slides are missing from the README. See inline comment →

Ignore files

The .prettierignore and .markdownlintignore additions are correct — Slidev's --- slide separators conflict with Prettier's markdown HR formatting and markdownlint rules. Good call excluding the presentations/ directory.

What looks good

  • The 300-line threshold for auto-review is a sensible filter to avoid noise on small PRs
  • The @claude trigger for on-demand review in comments is well-configured
  • The presentation content is technically accurate — the marker decision tree thresholds, iOS annotation manager pattern, Android batch update pattern, and MCP integration examples all match current Mapbox SDK patterns
  • Code examples in the slides correctly show anti-patterns (❌) vs solutions (✅), consistent with the repo's skill style

Comment on lines +17 to +18
actions: read
contents: read

Choose a reason for hiding this comment

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

The id-token: write permission is only needed for GitHub OIDC token exchange (GitHub App auth). Since this workflow uses ANTHROPIC_API_KEY directly (and the commit history suggests this was intentionally removed at one point), this permission can be dropped to follow least-privilege:

Suggested change
actions: read
contents: read
actions: read

if: github.event_name == 'pull_request'
id: pr-size
run: |
CHANGED_LINES=$(git diff --stat origin/${{ github.event.pull_request.base.ref }}...origin/${{ github.event.pull_request.head.ref }} | tail -1 | awk '{gsub(/[^0-9]/, " "); print $1 + $2}')

Choose a reason for hiding this comment

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

This awk extraction is fragile — git diff --stat summary format varies (e.g., " 8 files changed, 9953 insertions(+)" vs " 1 file changed, 3 insertions(+), 1 deletion(-)") and gsub(/[^0-9]/, " ") strips all non-digits then sums the first two numbers, which could pick up the file count instead of insertions.

A more robust approach using --numstat:

Suggested change
CHANGED_LINES=$(git diff --stat origin/${{ github.event.pull_request.base.ref }}...origin/${{ github.event.pull_request.head.ref }} | tail -1 | awk '{gsub(/[^0-9]/, " "); print $1 + $2}')
CHANGED_LINES=$(git diff --numstat origin/${{ github.event.pull_request.base.ref }}...origin/${{ github.event.pull_request.head.ref }} | awk '{ added += $1; removed += $2 } END { print added + removed }')

Do NOT flag minor prose style preferences as issues.

Provide detailed inline comments for specific issues.
claude_args: '--model claude-opus-4-6 --allowedTools "Bash(gh issue view:*),Bash(gh search:*),Bash(gh issue list:*),Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*),Bash(gh pr list:*),mcp__github_inline_comment__create_inline_comment,Read,Grep,Glob,Bash(git log:*),Bash(git diff:*),Bash(git show:*),Bash(git blame:*),Bash(git rev-parse:*),WebFetch(domain:github.com)"'

Choose a reason for hiding this comment

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

Nit: This line is 430+ characters. Consider using a YAML block scalar for readability and easier maintenance:

claude_args: >-
  --model claude-opus-4-6
  --allowedTools
  "Bash(gh issue view:*),Bash(gh search:*),..."


```yaml
---
name: mapbox-web-performance-patterns

Choose a reason for hiding this comment

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

The frontmatter mentions triggers in the prompt focus areas (line 48 of the workflow: "triggers, content"), but per CONTRIBUTING.md, the actual SKILL.md frontmatter schema only requires name and description — there's no triggers field. The trigger condition is embedded in the description text (e.g., "TRIGGER when...").

This slide's example is correct (just name + description), but note the workflow prompt references a field that doesn't exist in the schema.


## Slides

1. Title — Mapbox Agent Skills

Choose a reason for hiding this comment

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

The slide order listed here doesn't match the actual order in slides.md. For example, "Why Agent Skills?" and "Why Mapbox Needs Skills" appear before the deep dives in the actual deck but aren't listed here, while "Skills + MCP: Better Together" is in the deck but missing from this list. Consider regenerating this list from the actual slide headings to keep it accurate.

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