fix: add consistent bootc sudo policy - #932
Conversation
Assisted-by: GPT-5.6 Luna via GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
## Problem The `validate` check has been producing false-negative failures across the open PR backlog. At time of writing, 9 of 19 open PRs (#893, #917, #924, #926, #929, #931, #932, #933, #934) showed a red `validate` check unrelated to their own content: ``` error: docs/skills/index.json is stale. Run scripts/generate_skill_index.py --write ``` ## Root cause `scripts/generate_skill_index.py` stamped `generated_at` with `date.today()` at **script-execution time**, then byte-compared the result against the committed `docs/skills/index.json`. That means the check only passed if CI happened to run on the *same calendar day* the index was last regenerated on `main` — entirely independent of whether any skill doc actually changed. Any PR whose CI ran a day or more after the last regeneration failed. Reproduced locally with zero content edits: `--check` failed purely because days had passed since the last commit to `index.json`. This matters because `validate` is a **required status check** in the merge queue ruleset, so the noise trains reviewers to ignore a gate that is supposed to block merges. ## Fix Only advance `generated_at` when the catalog's actual content (`schema_version` + `skills`) differs from what is committed. `--write` becomes idempotent when nothing changed; `--check` tolerates a stale date as long as the skill data matches. ## Verification Proved in both directions, with a regression test for each: - **No false negative** — backdated the committed index to `2020-01-01` with zero skill content changes; `--check` now passes (previously failed). Covered by `test_generate_skill_index_check_tolerates_stale_generated_at`. - **Real drift still caught** — edited a skill's front matter without regenerating; `--check` still fails with `index.json is stale`. Covered by `test_generate_skill_index_check_still_fails_on_real_drift`. `just check` passes. Skill doc test suite: 8 passed. ## Alternatives rejected - *Compare against the merge result instead of PR head* — doesn't address the cause; the merge tree still gets stamped "today". - *Scope the check to changed files* — would blind it to a PR that edits a skill doc without regenerating the index. - *Stop committing generated files* — far larger blast radius for a date-comparison bug. - *Auto-commit bot step in CI* — added complexity over a small fix. Assisted-by: Claude Sonnet 5 via GitHub Copilot Co-authored-by: Jorge Castro <jorge@projectbluefin.io> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Breaking rewrite of pr-review.md from agent-review checklists (v2.0) to a human-decides, agent-lands backlog review workflow (v3.0). Key changes: - Remove agent-accelerated review lane (lines 31-56 of v2.0) - Encode dossier → verdict → stage → land loop with batch size of 5 - Add per-PR human verdict vocabulary (merge/close/defer/rebase/changes/open/skip) - Add issue triage sweep with issue-specific verdicts - Add blast radius map with system_files/shared/ never-fast-lane rule - Default to merge queue (--auto), --admin requires explicit human instruction - Include worked example using real backlog PRs (#926, #932, #933, #934, #936) - Extract per-type review checklists to See Also links (existing skill docs) - State verified merge queue config (squash-only, ALLGREEN) without restating review count (known doc drift with governance.md, being fixed separately) 296 lines (was 440). Per-type checklists (systemd, shell, dconf, OEM, test, CI) are no longer in this file — the human does that judgment using the linked per-domain skill docs. Assisted-by: Claude Opus 4.6 via GitHub Copilot Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Deferred to the human queue during backlog triage — this is sudo policy on The problem is real. Confirmed on a current image that As written, it does not fix this repo's own call site.
The rule pins # system_files/shared/usr/share/ublue-os/just/default.just:145
$(sudo bootc status --verbose)
Checked and clear:
Open decisions for a maintainer:
Re-derive: visudo -cf system_files/shared/etc/sudoers.d/001-bootc
grep -rn 'sudo bootc' system_files/shared/usr/share/ublue-os/just/
man 5 sudoers | grep -A6 'If a Cmnd has associated' |
hanthor
left a comment
There was a problem hiding this comment.
Requesting changes — the intent is right and the diff is one line, but as written the policy doesn't do what it says. Three concrete problems, all verifiable without a test system.
1. bootc update is not a subcommand. bootc's verbs are upgrade, switch, rollback, status, edit, install, container — there is no update (see bootc.8, which lists **bootc upgrade** | Download and queue an updated container image to apply). That clause is dead weight and grants nothing.
2. bootc status won't match the one place that actually uses it under sudo. sudoers argument matching is exact — a Cmnd with arguments matches only that exact argument vector. So /usr/bin/bootc status permits sudo bootc status and nothing else, while the repo's only sudo'd caller is:
system_files/shared/usr/share/ublue-os/just/default.just:145
$(sudo bootc status --verbose)
--verbose is an extra argument, so that still prompts for a password. The clause as written covers a call site that doesn't exist.
3. bootc status doesn't need root in the first place. This repo already proves it — bonedigger-report calls it unprivileged and treats failure as merely absent data:
system_files/bluefin/usr/libexec/bonedigger-report:82
BOOTC_JSON="$(bootc status --json 2>/dev/null || printf '{}')"
So the right fix for default.just:145 is to drop the sudo, not to grant a NOPASSWD rule for it.
Suggested change. Reduce the drop-in to the one verb that genuinely needs root and is genuinely invoked bare (update.just:26 is exactly sudo bootc upgrade):
%wheel ALL=(ALL) NOPASSWD: /usr/bin/bootc upgrade
and separately drop the sudo from default.just:145. If you do want status under sudo for some caller I've missed, it needs /usr/bin/bootc status * to tolerate flags — but I'd avoid the wildcard and just not sudo a read-only command.
Worth considering whether /usr/bin/bootc rollback belongs here too, since it's the natural partner to upgrade in a recovery flow and takes no arguments. Your call — out of scope for #142 if you'd rather keep it minimal.
One packaging check before merge: sudo ignores drop-ins that are group- or world-writable. Please confirm the OCI layer lands etc/sudoers.d/001-bootc as root-owned 0440 (or at least 0644) — if the build copies system_files/ with permissive modes, the file is silently skipped and the whole policy is a no-op with no error anywhere. just check and the json.tool/git diff --check validations in the PR body wouldn't catch that. visudo -cf on the file would be worth adding to CI given there's no other sudoers coverage in the repo.
Summary
Closes #142
Validation
just checkpython3 -m json.tool system_files/shared/etc/containers/policy.jsongit diff --checkvisudoandshellcheckwere not run because they are not installed in the checkout environment.