test: files/ payload reachability gate — tests/unit/test_files_payload_reachability.py - #155
Conversation
![sec-check[bot]](https://github.githubassets.com/images/gravatars/gravatar-user-420.png){kind=avatar}
…files_payload_reachability.py
Every path under files/ reaches an image only if some element stages it with a
kind: local source. Nothing cross-checks the two sides, so a payload directory
that no element names is invisible to `just validate` (which runs
`bst show --deps all` on the three oci/ targets only), to the image build, and
to the unit suite, because bats tests run helper scripts out of the checkout
whether or not they ship.
files/bin/system-container is the live instance: docs/skills/system-containers.md
promises it at /usr/bin/system-container in the OS image, it has bats coverage,
and no element stages it.
Add a gate asserting both directions of the contract:
- every kind: local `path:` declared by an element still exists on disk, so
renaming payload cannot silently drop it out of the image;
- every file under files/ is staged, declared host tooling (HOST_TOOLING:
files/bin/bluefin-kubestellar, files/lima), or a recorded waiver.
KNOWN_UNSTAGED holds exactly files/bin/system-container and is shrink-only:
staging a waived path fails the gate until the waiver is removed, so the record
cannot outlive the bug. The gate is structural only — no element, payload, or
documentation is changed, so /usr/bin/system-container is still absent from the
image until a maintainer decides to stage it or to correct the docs.
Refs #154
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Signed-off-by: sec-check[bot] <sec-check[bot]@users.noreply.github.com>
hivecommons-hive
Bot
added
hold
hivecommons-hive
Bot
changed the title
hivecommons-hive
Bot
added
the
agent/scanner
|
Deferring this PR's red Baseline evidence — the same check is failing on the default branch
This is one repository incident, not one failure per PR, so no PR-specific retry or diff repair is appropriate here. This PR stays deferred until
|
|
CI triage: the red check on this PR is not caused by its diff.
The console timeout is downstream of the It is already owned elsewhere — #167 ("mount /var via filesystem label and provide /bin/sh"), #165 ("cap root-a SizeMaxBytes to 8G to guarantee /var space") and #173 ("remove udevd sysext ordering drop-in to prevent var mount d…") all target this exact failure. Rather than write a fourth implementation of the same fix inside a test PR, this one stands down and waits: it should go green once whichever of those lands. No change pushed here; the reachability gate itself is unmodified.🐝 Hive Agent: — hive: agent=architect backend=copilot model=claude-opus-5 |
castrojo
left a comment
castrojo
left a comment
There was a problem hiding this comment.
Approving. The core invariant here is worth having and it is implemented honestly.
I checked the gate actually holds rather than assuming it does: I enumerated every kind: local declaration across elements/**/*.bst against the full files/ listing, and every payload file is genuinely accounted for — by a staged directory, by HOST_TOOLING, or by the single files/bin/system-container waiver. The test passes on the current tree for the right reason.
The strong part is the bidirectional check plus the shrink-only waiver enforcement. test_every_payload_file_is_staged_or_declared is the real gate — a new orphan under files/ fails it. test_declared_source_paths_exist catches rename drift in the other direction. test_known_unstaged_waivers_are_still_unstaged and test_host_tooling_declarations_are_not_stale are the ones I'd have asked for if they weren't here: they make a waiver unable to outlive the bug it was written for, which is a genuine invariant and the usual failure mode of waiver lists. And test_at_least_one_element_stages_payload is a legitimate anti-vacuity guard, because this gate's failure mode is passing silently.
I also like that the PR does not silently "fix" the files/bin/system-container orphan it discovered. Recording it as an explicit waiver rather than quietly staging it is the right instinct.
Two tests I'd prune, and one limitation I'd document. None blocks a tests-only PR, so I am approving rather than holding it, but they are worth a follow-up pass:
test_waivers_and_host_tooling_are_disjointasserts that two module-level literal sets defined forty lines above do not intersect. It cannot fail except by someone typing the same string into both constants in a single edit. That is a tautology over constants, not observable behaviour.test_files_tree_is_presentassertsfiles/exists and is non-empty. No plausible bug fails this and only this — a repo that deletedfiles/would fail the build,just validate, and the anti-vacuity guard immediately beneath it first.
The limitation worth a docstring sentence: _declared_paths() scans every .bst by text regardless of whether that element is reachable from the three OCI targets just validate builds. So an orphaned element still "declares" its path, and a file can be reported reachable while never actually shipping — which is a near neighbour of the bug class the docstring says it closes. State it as a known bound rather than leaving it implied.
Minor: test_declared_source_paths_exist is parametrized on sorted(_declared_paths()) and then calls _declared_paths() again inside the body purely to build the owners message, re-parsing every .bst once per case. Correct, just quadratic YAML parsing for cosmetics — a module-level cache would fix it.
Merge-order interaction worth watching, since this gate newly constrains anything that adds to files/: PR #80 adds new systemd preset payload alongside os-systemd-presets.bst, and a files/ path without a matching kind: local declaration will turn this gate red. #91 (adds files/k0s/sysext/k0s-kiosk-tls.service) and #90 (deletes a manifest) are both safe, since those directories are declared at directory level.
CI: build and unit pass. The only red check is installer-test, which fails on main itself at tip 5ebfeae (run 34920225526) and is unrelated to this change.
Needs one more independent approval under the two-reviewer policy.
Drop test_waivers_and_host_tooling_are_disjoint (a tautology over two module-level literal sets) and test_files_tree_is_present (subsumed by the anti-vacuity guard and the build itself). Record in the module docstring that staging is matched by declaration rather than by graph reachability from the OCI targets. Assisted-by: Claude Opus 4.6 via GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
castrojo
left a comment
castrojo
left a comment
There was a problem hiding this comment.
Re-approving after pushing a prune to this branch. My earlier approval was dismissed by that push, so this restores it, and I am explaining what I changed since I am not the author.
What I changed (commit adca828, 013a224..adca828, one file):
Removed two test functions that cannot fail:
test_waivers_and_host_tooling_are_disjoint— asserted thatHOST_TOOLING & KNOWN_UNSTAGEDis empty. Both are module-level literal sets declared about forty lines above the assertion, so nothing observable can break it; only someone typing the same string into both constants in a single edit could. A tautology over constants.test_files_tree_is_present— assertedfiles/exists and is non-empty. No plausible bug fails this and only this: a repo that deletedfiles/would fail the build,just validate, andtest_at_least_one_element_stages_payloaddirectly beneath it first.
I also added one sentence to the module docstring recording a real limitation that was implied but not stated:
Staging is matched by declaration, not by graph reachability: a path named by an element that no OCI target depends on still counts as staged here.
That matters because _declared_paths() scans every elements/**/*.bst by text regardless of whether the element is reachable from the three OCI targets just validate builds — so an orphaned element still "declares" its path, and a file can be reported reachable while never actually shipping. That is adjacent to the bug class the docstring says it closes, and it is better stated than discovered.
What I left untouched. All five load-bearing tests, verbatim: test_every_payload_file_is_staged_or_declared (the gate itself), test_declared_source_paths_exist (rename drift), test_known_unstaged_waivers_are_still_unstaged and test_host_tooling_declarations_are_not_stale (the shrink-only invariant, which is the part I most wanted kept — it stops a waiver outliving the bug it was written for), and test_at_least_one_element_stages_payload (anti-vacuity, legitimate here because this gate's failure mode is passing silently). Both module constants are retained; grep confirms the surviving tests still use them.
Verification, run independently of the change rather than taken on trust: python3 -m pytest tests/unit/test_files_payload_reachability.py -q gives 22 passed, down from 24 — exactly the two removed, neither parametrized. gh pr diff 155 --name-only lists only tests/unit/test_files_payload_reachability.py, and neither removed name appears in the file.
On the PR itself, unchanged from my earlier review: the bidirectional check plus shrink-only waiver enforcement is a genuine invariant worth having, and I particularly like that it records the files/bin/system-container orphan as an explicit waiver rather than quietly staging it to make the gate pass.
Merge-order note still stands: this gate newly constrains anything adding to files/, so #80 — which adds systemd preset payload — needs a matching kind: local declaration or it will turn this red.
CI: build and unit pass. The only red check is installer-test, which fails on main itself and is unrelated.
Needs one more independent approval under the two-reviewer policy.
|
CI triage: the red check is not caused by this PR, and nothing in this branch can turn it green. This PR adds exactly one file —
What the QEMU job actually failed onJob 105714003901, The console-readiness timeout is a symptom: It is pre-existing on
|
|
The red check on this PR is pre-existing breakage on This PR's diff is a single new file — Evidence that
In run 34920225526 the job breakdown is The remaining checks here are green: The installer-test job runs through the reusable Holding the branch as-is rather than rebasing onto a still-red
|
|
CI status re-verified. This branch already contains the current Evidence from the failing job (run 35375198501): The Two open PRs target this exact breakage: #173 ( Not removing the hold, and not papering over the QEMU failure from inside a unit-test file.🐝 Hive Agent: — hive: agent=architect backend=copilot model=claude-opus-5 copilot=1.0.78 |
|
CI triage for the red check on this PR — no change is warranted here. Failing check: This failure is pre-existing and repo-wide, not caused by this diff:
The installer-test failure is already occupied ground and is being worked separately — #181 (
|
|
CI status note (no code change pushed). The one red check on this PR is `installer-test / Install and boot in QEMU` (Build DDI artifacts). It is not caused by this branch:
No branch-local fix exists for this: the failure lives in the image boot path, and repairing it would mean a behaviour change to the shipped image, which is outside what this PR claims and outside what a hold-gated test-only PR should carry. Repairing the guest boot is already being worked separately (see #181 and #186), so this branch deliberately does not duplicate it. This PR stays as-is, hold intact, pending human review.🐝 Hive Agent: — hive: agent=architect backend=copilot model=claude-opus-5 copilot=1.0.78 |
|
CI triage: the red check on this PR is not caused by its diff.
The repair belongs to the boot/readiness path, not here — that ground is already claimed by open PRs (#181 readiness gate reachability, #205 console hydration retry, #173 udevd sysext ordering / var mount). Writing a second fix for it from this PR would duplicate them. No change pushed to this branch: there is nothing in its diff to repair. It should go green once the installer-test failure is fixed on `main`. Hold label untouched.🐝 Hive Agent: — hive: agent=architect backend=copilot model=claude-opus-5 copilot=1.0.78 |
|
CI status note — the red check on this PR is not caused by this diff. This PR adds exactly one file,
The single failure is
Both halves of the real fix are already claimed by other open PRs, so this branch deliberately does not touch them:
Pushing an installer or CI change onto this branch would duplicate that work and mix two unrelated clusters into one review, so this PR is left as-is. It should go green once #207 and #181 land and this branch is rebased. The
|
|
Verified against the Kubernetes cutover branch (#210) before merging: applied this patch on top of that tree and the gate passes 23/23, including the new |
1 participant
Refactor
Adds one file:
tests/unit/test_files_payload_reachability.py. No element,payload, workflow, or document is modified.
Cluster claimed
tests/unit/test_files_payload_reachability.pyelements/**/*.bst(parsed forkind: localpath:sources), the
files/tree listing.elements/bluefin-server/os-stack.bst,elements/oci/k0s-sysext.bst,files/k0s/sysext/k0s-manifests.conf,tests/unit/test_k0s_manifests.py,tests/unit/test_kubestellar_kiosk.py,.github/scripts/docs-checks.py,files/installer/repart.d/**,include/flatcar.yml,elements/flatcar/**,renovate.json,files/bin/**,docs/**.The structural problem
files/is the image-payload tree. A file reaches an image only when someelement stages it with a
kind: localsource:Nothing cross-checks the two sides. A directory under
files/that no elementnames is invisible to
just validate(which only runsbst show --deps allonoci/bluefin-server-ddi.bst,oci/bluefin-server-installer.bst, andoci/k0s-sysext.bst), invisible to the image build, and invisible to the unitsuite, because bats tests execute helper scripts straight out of the checkout
whether or not they ship. The reverse direction is equally unguarded: a
renamed payload directory leaves a stale
path:on the element and quietlyremoves the payload from the image.
files/bin/system-containeris the live instance —docs/skills/system-containers.mdcalls it "
/usr/bin/system-container... shipped in the OS image" (line 18)and "The OS image ships a small helper at
/usr/bin/system-container" (line 38),tests/unit/system-container_test.batscovers it, and no element stages it.Full analysis in #154.
What the gate asserts
test_declared_source_paths_exist— everykind: localpath:declared byany element still exists on disk (parametrized per declared path).
test_every_payload_file_is_staged_or_declared— every file underfiles/is staged by an element, declared host tooling, or a recorded waiver.
test_host_tooling_declarations_are_not_stale—HOST_TOOLING(
files/bin/bluefin-kubestellar,files/lima) still exists and is stillunstaged.
test_known_unstaged_waivers_are_still_unstaged—KNOWN_UNSTAGEDisshrink-only: staging a waived path fails the gate until the waiver is deleted,
so the record cannot outlive the bug it describes.
test_at_least_one_element_stages_payload— guards the gate itself, so aparser regression cannot make it vacuously pass.
KNOWN_UNSTAGEDholds exactly one entry,files/bin/system-container, citing#154. The gate therefore passes today and fails the moment a second orphan
appears.
Deliberately not fixed here
Staging
system-containerinto the image is a behaviour change (it adds abinary to the shipped OS) and correcting the skill is a documentation decision.
Both belong to a maintainer. This PR only makes the invariant checkable, which
is why it references #154 rather than closing it.
Verification
python3 -m pytest tests/unit -q→235 passed, 1 xfailed(was211 passed, 1 xfailed; +24 from this file, all parametrized or new).python3 .github/scripts/docs-checks.py→Docs checks passed.files/os/orphan/thing.conf→ fails withassert not ['files/os/orphan/thing.conf'];files/os/ssh→ fails withelements/bluefin-server/os-sshd-config.bst declares a kind: local source path that does not exist: files/os/ssh;files/bin/system-container→ fails withfiles/bin/system-container is now staged by an element. Remove it from KNOWN_UNSTAGED.Picked up automatically by
just test-unitand.github/workflows/unit-tests.yml;no workflow change needed.
Refs #154 — the gate lands here; deciding whether
system-containerships orthe docs are corrected is left open for maintainers.
Filed by architect agent (ACMM L5 — hold-gated mode). Hold-gated: human review required.
— hive: agent=architect backend=copilot model=claude-opus-5