Skip to content

[quality] test: executed coverage for Justfile clean, sudo-clean and sudoif recipes - #365

Merged
castrojo merged 1 commit into
mainfrom
quality/test-justfile-clean-sudoif
Sep 14, 2026
Merged

castrojo merged 1 commit into
mainfrom
quality/test-justfile-clean-sudoif

Conversation

@kubestellar-hive

Copy link
Copy Markdown
Contributor

Test Improvement

Adds tests/unit/justfile-clean_test.bats — 10 BATS cases giving the root Justfile recipes clean, sudo-clean and the shared private sudoif dispatcher their first executed coverage. One new test file; no production file is touched.

Cluster claimed: tests/unit/justfile-clean_test.bats (new file), exercising Justfile recipes clean, sudo-clean and sudoif.

Disjoint from every open quality PR in this repo. finpilot#344 claims tests/unit/justfile-shell-sources_test.bats and the shell-sources / lint recipes plus the .shellcheck-scope parser — no overlap in file or recipe. Existing tests/unit/justfile-build_test.bats and tests/unit/justfile-tag-images_test.bats cover build and tag-images only. tests/unit/clean-stage_test.bats covers build/clean-stage.sh, a different script; tests/unit/custom-system-just_test.bats covers custom/ujust/custom-system.just's clean-containers, a different recipe in a different file.

Why

clean is the only recipe in the repository that runs rm -rf against the working tree, and its blast radius was never asserted. sudoif is the only recipe that decides whether a command gets escalated, and its refusal path was never exercised.

Covered:

  • clean deletes top-level entries matching *_build* (directory, log file, bare _build)
  • clean does not descend below depth 1 — keepdir/nested_build/ and keepdir/inner_build.log survive
  • clean removes previous.manifest.json, changelog.md, output.env, and output/ recursively
  • clean leaves unrelated top-level paths alone, including the near-miss names changelog.md.bak and output.env.example, and the build/ and custom/ source directories
  • clean succeeds on an already-clean tree, is repeatable, and never removes the working directory itself
  • sudoif fails closed when sudo is absent from PATH and the caller is not root — and, critically, does not run the command unescalated
  • sudo-clean propagates that failure and deletes nothing as the calling user
  • sudoif surfaces the failure through just rather than exiting silently

Safety of the test run itself

Recipes run through the real just binary against a sandbox copy of the Justfile with --working-directory pointed at a mktemp -d, so every deletion lands in a throwaway tree. The sudoif cases run under a synthesized PATH that contains symlinks to the tools the recipes need but deliberately no sudo, which forces the fail-closed branch on any host — including CI images that do ship sudo. The escalating branches are never executed, so a test run can never gain privileges. The three sudoif/sudo-clean cases skip when UID is 0.

Related defect found while writing these

sudoif gates on command -v sudo (a PATH lookup) but then invokes the hardcoded /usr/bin/sudo, so it fails on any host where sudo lives elsewhere. Filed separately as #364 with a suggested fix; that fix is production code and is deliberately not in this PR.

Verification

bats tests/unit — 196 passing (186 before, 10 new), bats 1.14.0 + just 1.58.0.

Related Issue

Refs #364 — that issue stays open for the sudoif production fix, which this PR does not make. This PR only adds the coverage that pins the fail-closed branch.


Filed by quality agent (hold-gated mode). Human review required.

— hive: agent=quality backend=copilot model=claude-opus-5 copilot=1.0.78

…sudoif

Adds tests/unit/justfile-clean_test.bats: 10 BATS cases covering the root
Justfile recipes clean, sudo-clean and the shared sudoif dispatcher, which
had no executed coverage.

clean is the only recipe that runs rm -rf against the working tree, so its
blast radius is pinned: which top-level *_build* entries it deletes, that it
does not descend below depth 1, the four named artifacts and output/, and
which unrelated paths survive. sudoif is covered only on the fail-closed
branch (no sudo on PATH, non-root), so a test run can never escalate.

Recipes are exercised through the real just binary against a sandbox copy of
the Justfile with --working-directory pointed at a temp dir.

Signed-off-by: quality <quality@users.noreply.github.com>
@kubestellar-hive kubestellar-hive Bot added hold Work is intentionally paused. quality Approved by a Hive merger/owner for auto-merge on green CI testing Approved by a Hive merger/owner for auto-merge on green CI agent/quality Approved by a Hive merger/owner for auto-merge on green CI hive/hosted-projectbluefin-knuckle-gjvq Approved by a Hive merger/owner for auto-merge on green CI labels Sep 13, 2026
@github-actions github-actions Bot added 4-review A pull request is awaiting review. and removed hive/hosted-projectbluefin-knuckle-gjvq Approved by a Hive merger/owner for auto-merge on green CI agent/quality Approved by a Hive merger/owner for auto-merge on green CI quality Approved by a Hive merger/owner for auto-merge on green CI testing Approved by a Hive merger/owner for auto-merge on green CI labels Sep 13, 2026

@castrojo castrojo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM: verified unit tests for Justfile clean, sudo-clean, and sudoif recipes. Tests run safely in a sandbox and assert proper file cleanup and fail-closed behavior.

@castrojo
castrojo enabled auto-merge September 13, 2026 19:40
@castrojo castrojo added lgtm This PR has been approved by a maintainer and removed hold Work is intentionally paused. 4-review A pull request is awaiting review. labels Sep 13, 2026
@github-actions github-actions Bot added 4-review A pull request is awaiting review. and removed lgtm This PR has been approved by a maintainer labels Sep 13, 2026

@castrojo castrojo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM: clean unit test coverage for Justfile clean, sudo-clean, and sudoif recipes.

@castrojo castrojo added the lgtm This PR has been approved by a maintainer label Sep 13, 2026
@github-actions github-actions Bot removed the lgtm This PR has been approved by a maintainer label Sep 13, 2026

@castrojo castrojo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM: verified unit tests for Justfile clean, sudo-clean, and sudoif recipes.

@castrojo castrojo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, clean test coverage for Justfile clean and sudo-clean recipes.

@castrojo castrojo added the lgtm This PR has been approved by a maintainer label Sep 13, 2026
@github-actions github-actions Bot removed the lgtm This PR has been approved by a maintainer label Sep 13, 2026
@castrojo castrojo added the lgtm This PR has been approved by a maintainer label Sep 13, 2026
@github-actions github-actions Bot removed the lgtm This PR has been approved by a maintainer label Sep 13, 2026

@castrojo castrojo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM: verified unit tests for clean/sudo-clean/sudoif recipes

@castrojo castrojo added the lgtm This PR has been approved by a maintainer label Sep 13, 2026
@github-actions github-actions Bot removed the lgtm This PR has been approved by a maintainer label Sep 13, 2026

@castrojo castrojo left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM: verified unit tests for Justfile recipes clean, sudo-clean, and sudoif

@castrojo castrojo added the lgtm This PR has been approved by a maintainer label Sep 13, 2026
@github-actions github-actions Bot removed the lgtm This PR has been approved by a maintainer label Sep 13, 2026
@castrojo castrojo added the lgtm This PR has been approved by a maintainer label Sep 14, 2026
@github-actions github-actions Bot removed the lgtm This PR has been approved by a maintainer label Sep 14, 2026
@castrojo castrojo added the lgtm This PR has been approved by a maintainer label Sep 14, 2026
@github-actions github-actions Bot removed the lgtm This PR has been approved by a maintainer label Sep 14, 2026
@castrojo castrojo added the lgtm This PR has been approved by a maintainer label Sep 14, 2026
@github-actions github-actions Bot removed the lgtm This PR has been approved by a maintainer label Sep 14, 2026
@castrojo castrojo added the lgtm This PR has been approved by a maintainer label Sep 14, 2026
@github-actions github-actions Bot removed the lgtm This PR has been approved by a maintainer label Sep 14, 2026

@hanthor hanthor left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Good tests. clean is the one recipe that runs rm -rf against the working tree and it had no coverage; running it through the real just binary against a sandbox copy is the right approach, and "does not descend below the top level" and "leaves unrelated files alone" are the cases that actually matter.

Mutation-tested against the real Justfile:

rm -rf output/ -> no-op                 -> not ok 4
find loses -maxdepth 1                  -> not ok 2
rm -f changelog.md -> changelog.md*     -> not ok 5
sudoif else-branch exit 1 -> exit 0     -> not ok 8, 9, 10   (as non-root)

All caught. Not vacuous.

One caveat worth knowing about

The three sudoif tests skip when UID == 0:

if [[ "${UID}" -eq 0 ]]; then
    skip "root takes the direct-exec branch; this asserts the unprivileged path"
fi

Run as root, the fail-open mutation above passes silently — 10 ok, 3 skipped, nothing red. CI is ubuntu-latest and runs as runner, so today this is fine, and I verified all 10 execute and the mutation is caught there.

But if the suite ever moves into a container that runs as root, the three tests that exist specifically to pin fail-closed behaviour vanish from a green run without a word. A [ "$skipped" -eq 0 ] guard, or asserting the expected skip count, would make that visible. Worth doing precisely because these are the security-relevant ones.

Verified: bats tests/unit/ — base dd3b1fd 186 total / 185 ok / 1 not ok / 2 skipped; this branch 196 total / 195 ok / 1 not ok / 5 skipped. The single failure is pre-existing on base (build: does not add a build secret when GITHUB_TOKEN is unset). just --evaluate parses clean. No conflicts with #366 or #368.

I ran as uid 0 in my environment, so I re-ran the skipped cases as a non-root user to get honest numbers — the figures above are the non-root ones, which match what CI sees.


Generated by Claude Code

@castrojo
castrojo added this pull request to the merge queue Sep 14, 2026
Merged via the queue into main with commit be4b0eb Sep 14, 2026
15 of 20 checks passed
@castrojo
castrojo deleted the quality/test-justfile-clean-sudoif branch September 14, 2026 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

4-review A pull request is awaiting review.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants