Repository navigation
ci: add routed Lua, Python, workflow and package checks - #16
Conversation
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.
|
@pullfrog review |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
gpt-6.1-sol | 𝕏
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.
|
@pullfrog review |
There was a problem hiding this comment.
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
mainruns 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.
gpt-6.1-sol | 𝕏
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.

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
mainpushes 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: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:
Bug Fixes:
Enhancements:
CI:
Documentation:
Tests: