Skip to content

πŸ“Š [gha] add godot-specific checks#4

Merged
chicks-net merged 3 commits intomainfrom
chicks/2026-01-04-godot-checks
Jan 4, 2026
Merged

πŸ“Š [gha] add godot-specific checks#4
chicks-net merged 3 commits intomainfrom
chicks/2026-01-04-godot-checks

Conversation

@chicks-net
Copy link
Member

@chicks-net chicks-net commented Jan 4, 2026

Done

  • πŸ“Š [gha] add godot-specific checks
  • fix global GHA permissions
  • rename to Godot Checks visibly to match filename

Meta

(Automated in .just/gh-process.just.)

Copilot AI review requested due to automatic review settings January 4, 2026 06:00
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR introduces a GitHub Actions workflow to enforce code quality standards for Godot projects. The workflow runs static analysis tools including GDScript formatting, linting, and spell checking on relevant project files.

Key Changes

  • Adds automated GDScript formatting validation using gdformat
  • Implements GDScript linting via gdlint
  • Integrates spell checking with codespell for documentation and code

πŸ’‘ Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

uses: codespell-project/actions-codespell@v2
with:
skip: ./addons,*.po
ignore_words_list: chello,doubleclick,Manuel, SectionIn
Copy link

Copilot AI Jan 4, 2026

Choose a reason for hiding this comment

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

Inconsistent spacing in the ignore_words_list. There's a space after 'Manuel,' but no spaces after other commas. For consistency, remove the space: 'chello,doubleclick,Manuel,SectionIn'.

Suggested change
ignore_words_list: chello,doubleclick,Manuel, SectionIn
ignore_words_list: chello,doubleclick,Manuel,SectionIn

Copilot uses AI. Check for mistakes.
@claude
Copy link

claude bot commented Jan 4, 2026

Code Review: Godot Static Checks Workflow

Summary

This PR adds a GitHub Actions workflow for Godot-specific static checks including GDScript formatting, linting, and spell checking. Overall, this is a solid addition that will help maintain code quality as the project develops.

βœ… Strengths

  1. Appropriate tooling: Using godot-gdscript-toolkit is the right choice for GDScript linting and formatting
  2. Efficient path filtering (.github/workflows/godot-checks.yml:7-9): Only runs when addons/** or scripts/** change, avoiding unnecessary workflow runs
  3. Concurrency control (.github/workflows/godot-checks.yml:11-13): Good use of concurrency groups to cancel in-progress runs
  4. Spell checking: Including codespell is a nice touch for catching typos

⚠️ Issues & Recommendations

1. Critical: Missing permissions block

All other workflows in this repo (checkov.yml, actionlint.yml, markdownlint.yml) explicitly declare permissions with a global permissions: {} and job-level granular permissions. This workflow should follow the same security pattern:

# global permissions
permissions: {}

jobs:
  static-checks:
    runs-on: ubuntu-latest
    permissions:
      contents: read  # for actions/checkout
    steps:
      # ...

This follows the principle of least privilege and matches the repository's established security standards.

2. Action pinning inconsistency

  • .github/workflows/godot-checks.yml:20 uses actions/checkout@v6
  • Other workflows use actions/checkout@v4 (checkov.yml:32, actionlint.yml:22, markdownlint.yml:18)

While v6 may be newer, consistency across the repo is valuable. Consider:

  • Either update all workflows to v6 (separate PR), or
  • Use v4 here to match existing patterns

3. Using branch instead of version tag

.github/workflows/godot-checks.yml:21 uses Scony/godot-gdscript-toolkit@master

This is a security and reproducibility concern:

  • master branch can change at any time, potentially breaking your CI or introducing malicious code
  • Best practice is to pin to a specific version tag or commit SHA
  • Check if this action has version tags; if not, consider using a commit SHA

4. Directories don't exist yet

The workflow filters on addons/** and scripts/** paths, but these directories don't currently exist in the repository. While this is forward-looking, consider:

  • Is this premature? Should you wait until you actually have GDScript files to check?
  • Or is this intentional preparation for upcoming work?

If intentional, this is fine, but the workflow will simply skip on all current commits.

5. Spell check configuration concern

.github/workflows/godot-checks.yml:29-30: The ignore_words_list contains Manuel - this looks like it might be a proper name. Consider whether this belongs in a project-specific .codespellrc file instead of the workflow, for better maintainability.

Also, there's a trailing space after Manuel, in the ignore_words_list which might cause parsing issues.

6. No failure handling

Unlike checkov.yml which uses if: success() || failure() for certain steps, this workflow has no error handling strategy. Consider:

  • Should formatting failures block PRs completely, or should they be warnings?
  • Should spell check failures be blocking?

πŸ”’ Security Considerations

πŸ“‹ Test Coverage

This workflow itself has no tests, which is typical for GitHub Actions. However:

  • The workflow will be tested by actionlint.yml
  • Consider testing it manually on a branch with sample GDScript files

🎯 Recommendations Priority

  1. HIGH: Add explicit permissions block
  2. HIGH: Pin godot-gdscript-toolkit to a version/SHA instead of @master
  3. MEDIUM: Align checkout action version with other workflows
  4. LOW: Clean up ignore_words_list (remove trailing space after Manuel)
  5. LOW: Consider moving spell check config to .codespellrc file

πŸ“ Suggested Fix

Here's a suggested improved version addressing the high-priority issues:

name: Static Checks πŸ“Š
on:
  push:
    branches-ignore:
      - gh-pages
  pull_request:
    paths:
      - "addons/**"
      - "scripts/**"

# global permissions
permissions: {}

concurrency:
  group: ci-${{github.actor}}-${{github.head_ref || github.run_number}}-${{github.ref}}-static
  cancel-in-progress: true

jobs:
  static-checks:
    runs-on: ubuntu-latest
    permissions:
      contents: read  # for actions/checkout to fetch code
    steps:
      - name: Checkout
        uses: actions/checkout@v4
      - uses: Scony/godot-gdscript-toolkit@<version-tag-or-sha>
      - name: Formatting checks
        run: gdformat --diff .
      - name: Linting checks
        run: gdlint .
      - name: Spell checks via codespell
        uses: codespell-project/actions-codespell@v2
        with:
          skip: ./addons,*.po
          ignore_words_list: chello,doubleclick,Manuel,SectionIn

✨ Overall Assessment

This is a good addition that demonstrates forward-thinking about code quality. With the security fixes applied (permissions and pinning), this will be a solid workflow. The main concern is following the repository's established security patterns.


πŸ€– Generated by Claude Code Review

@claude
Copy link

claude bot commented Jan 4, 2026

Code Review - PR #4: Add Godot-specific checks

Summary

This PR adds a GitHub Actions workflow for Godot-specific static checks including GDScript formatting, linting, and spell checking. Overall, the implementation is solid with good security practices (global permissions set to {}).

Issues & Recommendations

πŸ”΄ Critical Issues

  1. Missing addons/ and scripts/ directories (.github/workflows/godot-checks.yml:7-9)

    • The workflow filters on addons/** and scripts/** paths, but these directories don't exist in the repository yet
    • This means the workflow will never run until Godot code is added
    • Recommendation: Either remove the path filters for now, or create placeholder directories with .gitkeep files
  2. Inconsistent checkout action version (.github/workflows/godot-checks.yml:23)

    • Uses actions/checkout@v6 while other workflows use @v4 (see markdownlint.yml:18, actionlint.yml:22)
    • Recommendation: Standardize on @v4 for consistency, or update all workflows to @v6

🟑 Medium Priority Issues

  1. Using @master for third-party action (.github/workflows/godot-checks.yml:24)

    • Scony/godot-gdscript-toolkit@master pins to a moving target, which is a security and stability risk
    • Best practice is to pin to specific commit SHA or version tag
    • Recommendation: Pin to a specific version: Scony/godot-gdscript-toolkit@<commit-sha> or use a release tag if available
  2. Missing job-level permissions (.github/workflows/godot-checks.yml:19)

    • Global permissions are set to {} (good!), but the job doesn't declare contents: read
    • Other workflows (markdownlint.yml:16, actionlint.yml:20) follow this pattern
    • Recommendation: Add job-level permissions:
    static-checks:
      runs-on: ubuntu-latest
      permissions:
        contents: read
  3. Trailing space in ignore_words_list (.github/workflows/godot-checks.yml:33)

    • ignore_words_list: chello,doubleclick,Manuel, SectionIn has a space after "Manuel,"
    • May cause unexpected behavior
    • Recommendation: Remove space: ignore_words_list: chello,doubleclick,Manuel,SectionIn

🟒 Minor Issues / Suggestions

  1. Missing workflow documentation

    • No comments explaining what gdformat/gdlint do or why specific words are ignored
    • Recommendation: Add brief comments for future maintainers
  2. Branch filter inconsistency

    • Uses branches-ignore while other workflows use branches (actionlint.yml:6-8)
    • Both are valid, but consistency improves maintainability
    • Recommendation: Consider using branches: ["main"] on push events to match other workflows

βœ… What's Done Well

  • Global permissions set to {} (addresses the "fix global GHA permissions" commit)
  • Concurrency control with cancel-in-progress: true (efficient resource usage)
  • Good separation of concerns (formatting, linting, spell checking as separate steps)
  • Uses established tools (codespell, gdlint, gdformat)

Testing Recommendation

Since there are no .gd files yet, this workflow won't be triggered. Consider:

  • Testing the workflow manually with workflow_dispatch trigger, or
  • Adding sample Godot files to verify the workflow works as expected

Security Assessment

βœ… No security concerns. The global permissions fix is correctly implemented.


Review provided by Claude Code

@claude
Copy link

claude bot commented Jan 4, 2026

Code Review - PR #4

Summary

This PR adds Godot-specific CI checks for GDScript formatting, linting, and spell checking. The workflow is well-structured and follows repository conventions.

Positive Aspects

  1. Security Best Practices βœ…

    • Correctly implements global permissions: {} with no job-level overrides needed
    • Follows the security pattern established in other workflows (.github/workflows/checkov.yml:15, .github/workflows/markdownlint.yml:10)
  2. Appropriate Triggers βœ…

    • branches-ignore: gh-pages prevents unnecessary runs on documentation branches
    • Path filtering (addons/**, scripts/**) ensures the workflow only runs when relevant Godot files change
    • Runs on both push and pull_request for comprehensive coverage
  3. Concurrency Control βœ…

    • Proper concurrency group with cancel-in-progress: true prevents wasted CI resources
    • Unique group identifier prevents conflicts with other workflows
  4. Clear Naming βœ…

    • Workflow name "Godot Checks πŸ“Š" matches the filename godot-checks.yml (per commit 3)
    • Descriptive step names make the workflow easy to understand

Issues & Recommendations

πŸ”΄ Critical: Pinning Third-Party Action to Master Branch

Location: .github/workflows/godot-checks.yml:24

- uses: Scony/godot-gdscript-toolkit@master

Issue: Using @master is a security and stability risk. The master branch can change at any time, potentially:

  • Introducing breaking changes that fail your CI
  • Containing malicious code if the upstream repo is compromised
  • Making builds non-reproducible

Recommendation: Pin to a specific commit SHA or version tag:

- uses: Scony/godot-gdscript-toolkit@<commit-sha>

Check the repository for available releases/tags and use the most recent stable version.

⚠️ Medium: Missing Job-Level Permissions

Location: .github/workflows/godot-checks.yml:18-20

While the workflow correctly sets global permissions: {}, the godot-checks job should explicitly declare the permissions it needs.

Current:

jobs:
  godot-checks:
    runs-on: ubuntu-latest

Recommended:

jobs:
  godot-checks:
    runs-on: ubuntu-latest
    permissions:
      contents: read  # for actions/checkout

This follows the pattern in markdownlint.yml:15-16 and makes permissions explicit.

⚠️ Medium: Checkout Action Version Mismatch

Location: .github/workflows/godot-checks.yml:23

- uses: actions/checkout@v6

Issue: Other workflows in the repository use @v4 (see checkov.yml:32, markdownlint.yml:18). While v6 may be intentional for future-proofing, this inconsistency should be addressed.

Recommendation: Either:

  1. Use @v4 for consistency with existing workflows, OR
  2. Update all workflows to @v6 in a separate PR

πŸ’‘ Minor: Codespell Configuration

Location: .github/workflows/godot-checks.yml:32

skip: ./addons,*.po

Issue: The skip parameter uses ./addons (relative path with ./) which may behave inconsistently.

Recommendation: Use consistent path format:

skip: addons,*.po

Also, the ignore_words_list contains Manuel (capitalized), which seems like it might be a typo or project-specific name. Consider documenting why these specific words are ignored (either in the workflow comments or in a .codespellrc file).

πŸ’‘ Minor: Path Filters May Be Too Restrictive

Location: .github/workflows/godot-checks.yml:7-9

paths:
  - "addons/**"
  - "scripts/**"

Question: Should the workflow also run when the workflow file itself changes? Consider adding:

paths:
  - "addons/**"
  - "scripts/**"
  - ".github/workflows/godot-checks.yml"

This ensures changes to the CI configuration are validated.

Test Coverage

Gap: The repository currently has no Godot files (no .gd files found, no addons/ or scripts/ directories exist yet). This means:

  • The workflow cannot be tested until Godot code is added
  • The path filters are untested
  • The tools (gdformat, gdlint, codespell) haven't been validated against actual code

Recommendation: Consider adding a simple test fixture or waiting to merge this until there's actual Godot code to validate against.

Overall Assessment

Status: Approve with minor changes recommended

The workflow is well-designed and follows repository conventions. The critical issue (unpinned @master action) should be fixed before merging. The other suggestions improve consistency and security but aren't blockers.

Priority Fixes:

  1. Pin Scony/godot-gdscript-toolkit to a specific version/SHA
  2. Add explicit permissions: contents: read to the job
  3. Align actions/checkout version with other workflows

Review generated by Claude Code

@chicks-net chicks-net merged commit 066a812 into main Jan 4, 2026
6 checks passed
@chicks-net chicks-net deleted the chicks/2026-01-04-godot-checks branch January 4, 2026 06:11
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