Clear the findings that stand in the way of linting in CI - #9
Merged
Merged
Conversation
added 2 commits
September 8, 2026 15:16
Preparation for switching on CI checks: every linter that is worth running here reports findings on the current tree, so a workflow added today would be red from the first run. This clears them, so the workflow can start green and stay meaningful. Shell (shellcheck -x -S warning, 11 -> 0) - phobos-common.sh: quote the array subscripts passed to unset, otherwise a path containing glob characters would be expanded before removal. - phobos-common.sh is a library, so every variable it defines is read by the scripts that source it and never here. SC2034 is suppressed once for the whole file, with the reason stated, instead of twelve separate directives. - phobos-filesystem.sh: read the tail flags line by line into an array. The word splitting is intended, but now it is explicit and survives a multi-line file. - phobos_wrapper.sh: drop rlimit_arg. It was assembled but never used; "ulimit -v" a few lines further down does the actual limiting. C (cppcheck, 1 -> 0) - netcache_probe.c: the conditional promotes unsigned short to int, so "%u" received a signed value. Cast explicitly. Python (ruff 62 -> 0, bandit 1 high -> 0) - Applied ruff's automatic fixes (import splitting and ordering, PEP 585 annotations). - orchestrate.py: run() no longer accepts a command string and never uses a shell. Both call sites pass argument lists, so the string branch was dead code that only widened the injection surface. - The two broad "except Exception" clauses are deliberate and stay: one keeps a single failing language from aborting the other prune runs, the other is the fallback when the external realpath fails. Both are now marked with the reason. - The three scripts carry a shebang and are now executable. YAML (yamllint 13 -> 0) and Docker (hadolint, warnings -> 0) - Two files had CRLF line endings, which breaks tooling on Linux. - .yamllint relaxes the rules that buy nothing here (document start, 80 columns) but pins line endings to LF. - .hadolint.yaml ignores DL3008 with the reason: Ubuntu keeps only the current version of a package in its archive, so pinning apt versions breaks the build as soon as a security update ships. The base image tags are pinned instead and Renovate keeps them current. - .bandit skips B108: the /var/tmp paths are the agreed mount points inside the containers, not temporary files, and they are overridable defaults. No behaviour change intended. tests/timeout_units.sh (58 passed) and tests/network_cache_ports.sh (20 passed) both still pass.
Six parallel jobs, one per language plus the workflows themselves, so a failure points straight at the area that broke. Together they run in well under a minute. shell shellcheck -x -S warning c gcc -Wall -Wextra -Werror -fanalyzer, then cppcheck python ruff, then bandit yaml yamllint --strict docker hadolint workflows actionlint Conventions follow the Ares2 workflows: empty top-level permissions with a read-only grant per job, checkout without persisted credentials, actions pinned to a commit SHA, external binaries verified against a checksum, a timeout on every job, and concurrency cancellation for pull requests only. Some choices worth stating: - shellcheck runs with -x so it follows the "source" directives. Without it the shared library file is analysed in isolation and every caller reports findings that do not exist. - The C job discovers sources with find rather than listing them. A hard-coded list breaks as soon as a file is added or removed on another branch. - cppcheck runs with --enable=warning only. The "style" category is largely taste, and its findings differ between cppcheck versions, which would make the gate fail depending on the runner image rather than on the code. - bandit is configured through --ini: its --configfile expects YAML, while .bandit is an INI file, and passing the wrong one fails the run outright. - The thresholds and exceptions live in .hadolint.yaml, .yamllint and .bandit rather than in this workflow, so a local run behaves like CI. Every job was simulated locally against the actual commands before this was committed, and all six pass on this branch.
This was referenced Sep 8, 2026
MarkusPaulsen
pushed a commit
that referenced
this pull request
Sep 8, 2026
Two corrections to the previous commit, both found by comparing against the Ares2 originals rather than by reading my own version again. Section 4 had lost its fill-in scaffold. Ares2 ends the section with Prerequisites, Steps, Expected result, Negative case and the modes exercised, so an author is asked for the negative case in the body of the pull request, where a reviewer looks for it. My version dropped all of it and then reintroduced the same idea as a checklist tick, which is weaker: a tick claims the case was covered, the scaffold makes the author write down what it was. The scaffold is back, with the four Ares enforcement combinations replaced by the three layers this project composes, each of which can be disabled on its own. Section 4 had also lost both recurring meta-rules, its character limit and the "simple words" paragraph, even though the template states in its own header that they are repeated in every section where they apply. The counts now match Ares2 exactly: seven limits, six "simple words" paragraphs, eleven "always required" statements. The template check itself is adopted, which is what makes the template binding rather than advisory: - .github/scripts/CheckPullRequestTemplate.java differs from the Ares2 version in a single line, the escape phrase for section 5, because this repository has no production Java code to report coverage for and answers that section with "No behaviour covered by the suites changed". - .github/workflows/pullrequest-template.yml is taken over unchanged. It runs the checker in single-file source mode, so there is no build step, exempts Renovate and Dependabot at step level so a required check never hangs pending, and passes the pull request body through the environment rather than interpolating it into the shell, since a fork author controls it verbatim. Running the checker locally against the open pull requests: #11 and #12 pass, #8, #9 and #10 do not yet, and their descriptions are being brought into shape separately.
9 tasks
Add a lint workflow covering all five languages
added 2 commits
September 8, 2026 16:35
The CI failure on this branch comes from two changes that were each correct on their own. This branch adds a yamllint configuration with a 120 column limit, and the template check workflow was taken over from Ares2 unchanged, where one line is 151 columns. Ares2 does not lint YAML at all, so it never showed there. Neither side is wrong, so neither is weakened. The line is folded instead: a folded scalar joins its lines with single spaces, so the value handed to the workflow is character for character the one it was before. Verified by parsing the file and comparing the resulting string, not by reading it. Raising the limit or exempting .github/workflows would have hidden the same class of finding everywhere else, for one line. actionlint reports nothing on the folded file and yamllint is clean on the merge state of this branch with main, which is what CI checks and what the local branch alone does not reproduce: the file only exists on main, through the pull request that added the templates.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Groundwork for the lint workflow in #10. Every linter worth running here reports findings on the current tree, so a workflow added today would be red from its first run. This clears them: shellcheck 11 to 0, cppcheck 1 to 0, ruff 62 to 0, bandit 1 high to 0, yamllint 13 to 0, hadolint 3 warnings to 0.
Linked issues
No linked issues.
1. Problem
Nothing is broken at run time, this is a gap. The repository has no CI at all, and 2900 lines of shell, C and Python that define the sandbox boundary are unchecked.
Three of the findings are more than tidiness.
phobos-common.shcallsunseton array subscripts without quoting them, so a path containing glob characters would be expanded before removal, and paths here are attacker-influenced.orchestrate.pypassedshell=isinstance(cmd, str)tosubprocess.call, and this tool produces the sandbox allow-list.phobos_wrapper.shassembledrlimit_argand never used it.2. Improvement from the user's perspective
No Improvement from the user's perspective. This changes no behaviour a run can observe.
3. Improvement from the maintainer's perspective
The tree becomes lintable, which is what #10 builds on. The quoting fix removes a latent bug, and dropping the dead shell path in the orchestrator narrows the injection surface of the tool that produces the allow-list.
4. Testing manual
Prerequisites
Steps
find . -name '*.sh' -print0 | xargs -0 docker run --rm -v "$PWD:/mnt" -w /mnt koalaman/shellcheck:v0.11.0 -x -S warning.docker run --rm -v "$PWD:/repo" -w /repo ubuntu:24.04 bash tests/timeout_units.sh, thentests/network_cache_ports.shin the run-phase image.mainand repeat step 1.Expected result
Step 1 prints nothing and exits zero. Step 2 reports 58 passed, 0 failed, 0 skipped and 20 passed, 0 failed. Step 3 prints 11 findings, which is what this branch removes.
Negative case (what must still be rejected)
No behaviour of the sandbox changes here, so nothing that was denied before may be permitted now. The suites in step 2 assert exactly that pair and must stay green: they cover a denied port staying denied and a permitted port still reachable, and a malformed timeout aborting rather than running unlimited.
Layers exercised
No layer-specific behaviour changed.
5. Test case coverage regarding this PR
tests/timeout_units.shtests/network_cache_ports.shRun in an ordinary container from
ubuntu:24.04and the run-phase image, kernel 7.0-linuxkit, arm64, started without--privileged,--cap-addor--security-opt.Breaking changes and migration
No breaking changes or migration.
Checklist
--privileged,--cap-addor--security-opt, or the manual says why that was not possible.README.md, the comments incore/) was updated where the change is user-facing.Review progress