Skip to content

ci: add routed Lua, Python, workflow and package checks - #16

Merged
jatmn merged 3 commits into
mainfrom
codex/ci-lint-parity
Oct 8, 2026
Merged

jatmn merged 3 commits into
mainfrom
codex/ci-lint-parity

Conversation

@jatmn

@jatmn jatmn commented Oct 8, 2026 •

Copy link
Copy Markdown
Owner

Yuoki currently checks only the Pullfrog integration. This adds the lightweight CI already merged in Quinityn #10 and #11: complete Git-diff routing, Lua 5.2 syntax, Luacheck 1.2.0, StyLua 2.5.2, Python syntax/regressions, actionlint 1.7.12, and deterministic mod ZIP builds with exact content/license/checksum validation.

PRs and relevant main pushes check changed Lua files only until the deferred repository-wide baseline in #15 is completed. All 105 existing Lua files parse, but 99 need formatting and existing lint warnings remain. This PR changes no Lua gameplay source, assets, mod metadata or versions, and adds no blanket lint suppressions. Issue #15 remains open.

The existing Pullfrog authorization tests and actionlint gate are consolidated into the shared pipeline with their original workflow/helper-change triggers preserved. Its agent workflow, helper and tests remain unchanged. Checks have read-only repository permissions, run only affected jobs, cancel superseded PR runs, and never download Factorio or dependency mods. Contributor/development documentation records commands and the temporary enforcement policy.

The dispatcher executes the comparison revision's router so edited routing code cannot disable its own checks, and each non-PR run has independent concurrency. Both package tools reject nonregular or escaping sources (including metadata) and unsafe archive paths before reading them. The builder validates the metadata-derived name/version before opening the ZIP or writing the checksum, rejecting output-path escapes while preserving existing files. Syntax checks terminate compiler options for filenames beginning with -; repeated uploads replace the same run's artifact. Real before/after regressions cover these source, routing and compiler failures.

Validation on reviewed commit 9ed14241303bd36377ec7a9eba059495a201f6c4:

  • All seven tracked Python files parse; five real-Git CI-routing tests, six existing authorization subprocess tests and five package source-boundary tests pass. The compiler regression also runs explicitly in the Lua job.
  • actionlint 1.7.12, Lua configuration probes, whitespace checks and complete changed-hunk review pass.
  • Probes execute the actual Lua workflow blocks with real tools on both events. They cover unusual filenames, renames/deletions, both regular-file/symlink directions, untouched debt and empty/config-only changes; malformed syntax, unknown globals, bad format/config and missing revisions are rejected.
  • Metadata escape regressions reject both malformed name and version before output writes, preserving preexisting outside files and output ZIP/checksum bytes. They fail at the original head through the real CLI and pass after the fix.
  • Package validation passes for all 1,035 tracked release files, exact source bytes, required entrypoints, original license/acknowledgments, CRC and SHA256. Repeated builds have identical checksums; altered bytes, extra developer files, missing license and wrong checksum are rejected.

No Factorio runtime run is needed for this tooling-only change; these lightweight checks do not establish game API, save or graphical behavior. No branch-protection settings or release publication changed.

Hosted CI run 37814328633 passed on the reviewed head: change detection, Lua, Python, actionlint and packaging all succeeded. The unchanged upload replacement behavior was also verified by the second attempt of run 37811241991, which replaced the existing same-name artifact with a new ID.

Summary by Sourcery

Establish lightweight, change-routed CI for source validation, workflow checks, regression tests, and deterministic package verification without altering gameplay or release metadata.

New Features:

  • Add routed CI coverage for Lua, Python, workflow, and package validation on pull requests and main-branch pushes.
  • Add deterministic mod ZIP building with exact release-content, metadata, license, integrity, and checksum validation.

Bug Fixes:

  • Prevent edited CI routing code, unsafe package sources, and malformed archive paths from bypassing checks or escaping package boundaries.

Enhancements:

  • Consolidate Pullfrog authorization and workflow lint checks into the shared CI pipeline while preserving their existing triggers and read-only execution policy.
  • Document contributor validation commands, CI routing behavior, temporary changed-Lua enforcement, and packaging expectations.

CI:

  • Run only the validation jobs affected by the complete Git diff, with reusable workflows, cancellation of superseded PR runs, and independent main-branch concurrency.
  • Validate changed Lua files with Lua 5.2 syntax checks, Luacheck, and StyLua, alongside tracked Python syntax and regression tests and actionlint.

Documentation:

  • Add contributor and development documentation covering local checks, CI policy, package contents, and tooling limitations.

Tests:

  • Add regression coverage for Git-diff routing, unusual filenames, renames and deletions, file-type changes, router tampering, compiler failures, and package source-boundary violations.

Adapt the merged Quinityn CI tooling while preserving existing Pullfrog checks. Check changed Lua files on PRs and main pushes until the repository-wide baseline in #15 is complete.

Validated Python AST, three Git routing tests, six owner authorization tests, actionlint 1.7.12, real Lua 5.2/Luacheck 1.2.0/StyLua 2.5.2 workflow probes and deterministic 1035-file package validation. Gameplay sources and mod metadata are unchanged.
@jatmn jatmn self-assigned this Oct 8, 2026
@jatmn
jatmn marked this pull request as ready for review October 8, 2026 16:19
@jatmn

jatmn commented Oct 8, 2026

Copy link
Copy Markdown
Owner Author

@pullfrog review

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've found 4 issues

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="tools/package.py" line_range="28" />
<code_context>
+            entry = zipfile.ZipInfo(f'{name}/{file}', date_time=(2026, 10, 8, 0, 0, 0))
+            entry.compress_type = zipfile.ZIP_DEFLATED
+            entry.external_attr = 0o644 << 16
+            package.writestr(entry, (ROOT / file).read_bytes())
+    (output / 'SHA256SUMS').write_text(
+        f'{hashlib.sha256(target.read_bytes()).hexdigest()}  {target.name}\n'
</code_context>
<issue_to_address>
**Runner files enter package**

When a tracked release path is a symlink to a file outside the checkout that the runner can read, `pack` follows the symlink and writes the target’s bytes into the ZIP; `validate` follows it again and accepts those bytes, then the package workflow uploads the archive. A symlink to a readable file outside the checkout therefore exposes that file in the artifact.

Reject symlinks and require every packaged source to be a regular file contained within the checkout before reading it.

Also at `tools/validate_package.py:52`, `.github/workflows/package.yml:26`.
</issue_to_address>

### Comment 2
<location path="tools/package.py" line_range="25" />
<code_context>
+                continue
+            if any(part.startswith('.') or part in {'build', '__pycache__'} for part in relative.parts):
+                continue
+            entry = zipfile.ZipInfo(f'{name}/{file}', date_time=(2026, 10, 8, 0, 0, 0))
+            entry.compress_type = zipfile.ZIP_DEFLATED
+            entry.external_attr = 0o644 << 16
</code_context>
<issue_to_address>
**ZIP entries escape extraction root**

When a tracked filename contains backslashes and `..` components, `pack` places the tracked filename directly into the ZIP member name, while POSIX `Path` treats backslashes as ordinary characters. Windows extractors can interpret a filename containing backslash-separated `..` components as traversal and write outside the mod directory; `validate` accepts the same member name unchanged.

Reject archive member names containing backslashes or traversal components, and verify normalized entries remain beneath the archive root.

Also at `tools/validate_package.py:47`.
</issue_to_address>

### Comment 3
<location path=".github/workflows/ci.yml" line_range="36" />
<code_context>
+          BASE: ${{ github.event.pull_request.base.sha || github.event.before }}
+        run: |
+          git diff --check "$BASE" HEAD
+          python3 tools/ci_changes.py --base "$BASE" >> "$GITHUB_OUTPUT"
+
+  lua:
</code_context>
<issue_to_address>
**PR can skip every check**

When a pull request changes `tools/ci_changes.py` to emit `false` for every surface, `changes` executes the pull request’s version of `tools/ci_changes.py`, and the validation jobs trust the outputs it writes. A modified router can emit `false` for every surface and exit successfully, skipping all validation jobs; GitHub treats skipped jobs as successful checks.

Do not let PR-controlled routing code decide whether required checks run; base the router-change fallback on trusted workflow logic or require the relevant jobs independently.
</issue_to_address>

### Comment 4
<location path=".github/workflows/package.yml" line_range="22" />
<code_context>
+          python3 tools/package.py
+          python3 tools/validate_package.py
+      - name: Attach mod and checksum
+        uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4
+        with:
+          name: yuoki-${{ github.sha }}
</code_context>
<issue_to_address>
**Reruns fail to upload the package**

When a workflow or its jobs are rerun after an artifact with the same commit-based name has already been uploaded, the package job reuses `yuoki-${{ github.sha }}`, and `upload-artifact@v4` rejects the duplicate name, so the package job fails instead of attaching the rebuilt ZIP.

Set `overwrite: true` on the upload step or use an attempt-specific artifact name.

Also at `.github/workflows/package.yml:24-25`.
</issue_to_address>

Sourcery assessment

Needs a human reviewer. 4 findings to address first, and this adds a new CI dispatcher, reusable workflows, third-party actions, and a deterministic packaging path, so an incorrect routing or packaging decision could produce a wrong distributable artifact or allow a bad change through CI. The resulting artifacts and any merged code are not undone by reverting, but the bounded CI state can be corrected and artifacts regenerated on a subsequent run.

Blocking findings: tools/package.py:28, tools/package.py:25, .github/workflows/ci.yml:36, .github/workflows/package.yml:22


Sourcery is free for open source - if you like our reviews please consider sharing them ✨

Comment thread tools/package.py Outdated
Comment thread tools/package.py
Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/package.yml

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important

The main concurrency policy can cancel checks that the replacement run does not repeat. Fix that coverage gap before merging; a separate minor filename-handling issue is noted inline.

Reviewed changes All 16 changed files were reviewed, including the shared CI dispatcher, reusable validation workflows, packaging scripts, regressions, and contributor documentation.

  • Change routing: Complete Git-diff classification selects affected checks and preserves the Pullfrog authorization/workflow gates.
  • Lua validation: Lua 5.2 syntax, Luacheck, and StyLua enforce changed files while the repository-wide baseline remains deferred to issue #15.
  • Python and workflows: Tracked Python syntax and regression tests run alongside pinned actionlint validation.
  • Packaging and guidance: Deterministic ZIP creation and source/membership/license/checksum validation are documented without changing gameplay or release metadata.

Local validation passed all three routing tests, six authorization tests, syntax parsing of all six Python files, and .luacheckrc syntax. Packaging passed for 1,035 files, repeated builds were byte-identical, and altered content, extra developer files, missing license, and incorrect checksum were rejected. Both inline findings were reproduced locally; the hosted CI run also passed. No Factorio runtime validation was performed for this tooling-only change.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using gpt-6.1-sol | 𝕏

Comment thread .github/workflows/ci.yml Outdated
Comment thread .github/workflows/lua.yml Outdated
Use the base router to prevent edited routing code from skipping its own checks. Keep incremental main runs independent, handle option-like Lua filenames, and replace rerun artifacts.

Reject unsafe archive paths, nonregular sources and source escapes in both package tools, including metadata. Real regression tests fail at the original head and pass after repair; all local CI, syntax, authorization and 1035-file package gates pass. Whole-repository formatting remains deferred to #15.
@jatmn

jatmn commented Oct 8, 2026

Copy link
Copy Markdown
Owner Author

@pullfrog review

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Important

The package builder can overwrite a file outside its requested output directory before the validator rejects unsafe metadata. Fix the metadata-derived output path before merging; the earlier review findings are addressed.

Reviewed changes Reviewed all 17 changed files across both commits, including CI routing, reusable workflows, packaging, regressions, and contributor guidance.

  • CI routing: Complete Git-diff classification selects affected checks, executes the comparison revision's router, and preserves independent main runs alongside PR cancellation.
  • Lua checks: Lua 5.2, Luacheck, and StyLua enforce changed files, handle unusual filenames and type changes, and probe configuration while the full baseline remains deferred to issue #15.
  • Python and workflows: Tracked Python syntax and routing, authorization, and package-source regressions run alongside pinned actionlint validation.
  • Packaging and documentation: Deterministic tracked-source ZIP builds validate exact contents, notices, CRC, and checksums, reject unsafe source paths, and replace artifacts on reruns; contributor documentation explains the temporary enforcement policy.

Local validation passed five routing tests, six authorization tests, four package-source tests, syntax parsing of all seven tracked Python files, .luacheckrc syntax, actionlint 1.7.12, and whitespace checks. Packaging validated all 1,035 release files, and repeated builds produced identical SHA256 checksums. The inline finding was reproduced in an isolated fixture; hosted CI also passed on this head. No Factorio runtime validation was performed for this tooling-only change.

Pullfrog  | Fix all ➔ | Fix 👍s ➔ | View workflow run | Using gpt-6.1-sol | 𝕏

Comment thread tools/package.py
Apply the existing mod-name and version rules before deriving the ZIP destination or archive root. Reject escaping values before they can truncate files outside the requested output directory.

Real CLI regressions fail before the fix and pass after for both name and version escapes, preserving outside sentinels and existing ZIP/checksum output. All applicable local checks and 1035-file deterministic package validation pass; gameplay and mod metadata remain unchanged.
@jatmn
jatmn merged commit 0837648 into main Oct 8, 2026
6 checks passed
@jatmn
jatmn deleted the codex/ci-lint-parity branch October 8, 2026 17:19
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