Skip to content

fix(brain): align Ubuntu-only descriptions with action fences - #426

Merged
vladimirrott merged 4 commits into
lacs-project:mainfrom
QinXi-ai:fix/416-catalogue-fence
Sep 15, 2026
Merged

vladimirrott merged 4 commits into
lacs-project:mainfrom
QinXi-ai:fix/416-catalogue-fence

Conversation

@QinXi-ai

@QinXi-ai QinXi-ai commented Sep 14, 2026

Copy link
Copy Markdown
Contributor

Fixes #416.

Planner descriptions claimed Ubuntu exclusivity for Ufw and many other actions that the production execution fence permits elsewhere. Remove the stale claims across the catalogue and assert both directions of the relationship with UBUNTU_ONLY_ACTIONS, including presence of every fenced action in the catalogue. No action availability, execution policy, or public API changes.

Validation:

  • The new Rust regression compiled directly from the unchanged production constants and test body fails against the old descriptions (GetAptPins), then passes after the fix.
  • cargo fmt --all --check and git diff --check: PASS.
  • cargo test -p sysknife-brain --locked ubuntu_only_descriptions_match_execution_fence: BLOCKED on Windows by the existing std::os::unix::net::UnixDatagram import in journal.rs.
  • Linux CI on fc53c72 ran the complete workspace: 1,853 passed, 6 skipped. The only red step was the old 1,852 baseline. The official record_test_baseline.py recorder and all three prose files now use that observed count; it is a Linux CI measurement, not an estimated or local Windows count. Evidence: https://github.com/lacs-project/sysknife/actions/runs/34864948327/job/104046325126
  • Action reference regenerated from the production constructors and catalogue descriptions.
  • Final head 33285c6: Linux workspace passes all 1,853 tests (6 skipped), including the recorded-baseline check; Clippy, rustdoc, frontend, Postgres contract and hygiene gates pass. https://github.com/lacs-project/sysknife/actions/runs/34867797706

CI also surfaced the newly published RUSTSEC-2026-0285 in the inherited lockfile. A separate commit updates rustls to 0.23.45 and its required TLS dependencies; the final security-audit check passes.

Codex assisted with implementation and validation.

@vladimirrott vladimirrott 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.

Reviewed at 33285c61bae7f87e059d6a2092dbcfc44f7cb696.

Fifty-four action descriptions claimed "Ubuntu only" while the execution fence permitted them elsewhere. AptInstall on Debian 13 is not Ubuntu-only, and the model was being told it was. The fix is the right one and the regression is the right shape.

What matters most here is that your test reads the production constants rather than restating them:

for (name, description) in KNOWN_ACTIONS {
    assert_eq!(
        description.to_ascii_lowercase().contains("ubuntu only"),
        UBUNTU_ONLY_ACTIONS.contains(name),
        "{name}: Ubuntu-only description must match the execution fence"
    );
}
for name in UBUNTU_ONLY_ACTIONS {
    assert!(KNOWN_ACTIONS.iter().any(|(action, _)| action == name), ...);
}

UBUNTU_ONLY_ACTIONS comes from sysknife_core::action_family, so this is a real cross-check between two independent sources, not a parallel copy of the list it is checking. Both directions are covered, and both is_empty assertions in front stop it passing vacuously if either source goes empty. That last detail is the one people skip.

I proved it bites rather than taking the PR body's word for it. Restoring one stripped claim to GetAptPins, in a network-isolated container:

$ sed -i 's|show apt pin priorities (apt-cache policy) — param: package (optional); read-only|...; Ubuntu only; read-only|' \
    crates/sysknife-brain/src/planning_tools/propose_plan.rs
$ cargo test -p sysknife-brain --lib --offline ubuntu_only_descriptions_match_execution_fence
running 1 test
panicked at crates/sysknife-brain/src/planning_tools/propose_plan.rs:732:13:
assertion `left == right` failed: GetAptPins: Ubuntu-only description must match the execution fence
  left: true
 right: false
test result: FAILED. 0 passed; 1 failed

Unmutated, the same command reports running 1 test ... ok. The guard fails when the thing it protects is broken, and it names the offending action.

I checked the fence itself too. UBUNTU_ONLY_ACTIONS has ten entries and every one of them keeps its "Ubuntu only" text; all 54 you stripped live in DEBIAN_ONLY_ACTIONS or one of the NON_CANONICAL_ON_* sets. The realignment is factually correct, not just internally consistent. The Distro column in the generated reference is untouched, which is right, since that derives from action_family and this PR does not change availability.

Test count handled properly: one test added, 1852 to 1853, and all four surfaces moved together.

Your lockfile commit is also doing real work beyond this PR. RUSTSEC-2026-0285 against rustls published today and main at 61b3a878 is red on cargo audit because of it. I confirmed your head clears it:

$ git checkout --detach p413/p426 lockfile state; cargo audit
cargo audit rc=0

Since this is the only one of your four open PRs that does not touch .github/workflows/**, it is the one I can land without a second pair of human eyes on a workflow diff, and landing it takes the advisory off main for everyone else. So this goes first.

Approving, and merging through the gate.

@vladimirrott
vladimirrott merged commit 4b7b5d5 into lacs-project:main Sep 15, 2026
12 checks passed
@vladimirrott

Copy link
Copy Markdown
Member

Merged as 4b7b5d5.

The direction you chose is the part worth naming. Ten action names sit in UBUNTU_ONLY_ACTIONS (crates/sysknife-core/src/action_family.rs:142-153), and dozens of catalogue descriptions claimed "Ubuntu only" for actions that fence never covered. Widening the fence to match the prose would also have made the mismatch go away, and it would have changed what the daemon refuses to run on a Debian host. You narrowed the prose instead, and then wrote the test that keeps the two from drifting apart again.

The merge gate ran the proof rather than accepting my summary of it:

$ maintainer-merge verify 426 33285c61bae7f87e059d6a2092dbcfc44f7cb696 'ubuntu_only_descriptions_match_execution_fence' 's/list installed snaps — no params; read-only/list installed snaps — no params; Ubuntu only; read-only/' rust
  rust suite: evidence of 1 executed unit(s), passing unmutated
  applying the mutation and re-running
  receipt recorded for #426 at 33285c61 (observed: clean pass, mutated fail)

The mutation puts one Ubuntu only back onto SnapList, which is not among the ten fenced names, and your guard catches it at rc=101:

test planning_tools::propose_plan::tests::ubuntu_only_descriptions_match_execution_fence ... FAILED

Your lockfile commit cleared the advisory off main in the same merge:

$ grep -A1 '^name = "rustls"$' Cargo.lock
name = "rustls"
version = "0.23.45"
$ cargo audit
rc=0

RUSTSEC-2026-0285 had security-audit red on main and on every open pull request that did not carry your bump. #377 and #401 get a green board out of this, which is worth more than the fence fix on its own.

Two knock-on effects, both mine and neither yours:

You are already holding #417, #416, #346 and #239, so I am not putting another issue in front of you.

You have touched the action catalogue, the sudoers grants and the CI gate inside one week, which is most of the surface an operator ever sees. If you run Ubuntu anywhere that matters, a server you patch or a box carrying real traffic, I would like to know how SysKnife reads there. npx sysknife-setup gets it running, and the read-only side (sysknife doctor, sysknife history, sysknife audit verify) changes nothing on the host.

vladimirrott added a commit to Georgefifth/sysknife that referenced this pull request Sep 15, 2026
…and lacs-project#421

The branch recorded 1853, which was right when it was written and is one short
now that lacs-project#426 landed. The figure comes from the runner rather than from
arithmetic:

    $ UPDATE_TEST_BASELINE=1 scripts/test_baseline.sh
    test_baseline: recorded 1854 rust tests in tests/evidence/workspace-tests.json

The three published figures move with it, in README.md, docs/introduction.md and
docs/distro-support.md, and scripts/check_evidence_claims.py agrees:

    Published figures match the evidence artifacts.

Maintainer change on a contributor branch; the pull request's own commits are
untouched.
vladimirrott added a commit that referenced this pull request Sep 15, 2026
* fix(daemon): declare CHAIN_ROW_COLUMNS once, in store.rs

The 17-column ChainRow SELECT list was declared twice — once per backend
— kept in sync only by a 'Mirrors' doc comment. The Postgres copy's own
comment records the last drift: a column addition updated one query and
the mapper, and the miss surfaced only as a runtime 'no column found for
name: chain_version' from the live-Postgres test while unit tests stayed
green. Hoist one pub(crate) const into store.rs, the shared parent of
both backends; both modules import it, the four SELECT sites keep their
per-backend placeholders (?1 / $1), and the second declaration goes.

The drift test ran green first, was mutated red on purpose (failure
pasted in the PR), and is replaced by a pin on the exact 17-column list
— the invariant both mappers depend on.

Closes #397

* chore: move the rust test baseline to 1853 with the new pin

chain_row_columns_pinned adds one test to the workspace suite. The
baseline and the three published figures that derive from it move with
it, per the check the CI run called out.

* chore(evidence): move the rust baseline to 1854 after #426 and #421

The branch recorded 1853, which was right when it was written and is one short
now that #426 landed. The figure comes from the runner rather than from
arithmetic:

    $ UPDATE_TEST_BASELINE=1 scripts/test_baseline.sh
    test_baseline: recorded 1854 rust tests in tests/evidence/workspace-tests.json

The three published figures move with it, in README.md, docs/introduction.md and
docs/distro-support.md, and scripts/check_evidence_claims.py agrees:

    Published figures match the evidence artifacts.

Maintainer change on a contributor branch; the pull request's own commits are
untouched.

---------

Co-authored-by: Vladimir Rotariu <sysknife-development@protonmail.com>
vladimirrott added a commit to QinXi-ai/sysknife that referenced this pull request Sep 15, 2026
Three conflicts, all from my merge order rather than from this branch.

sudoers_grants_match_argv.rs: lacs-project#421's `checked += 1` and this branch's
shell-launcher assertion were inserted at the same point. Both kept, the counter
first so every examined spec is counted before the assertion can panic. The test
compiles and passes: 2 passed, 0 failed.

CHANGELOG.md: a `### Changed` section against main's `### Fixed`. Both kept,
Changed first per Keep a Changelog ordering.

docs/action-reference.md: not hand-merged. The file is generated, and the two
sides were different generations: this branch rewrote the command column to the
action-steps helper, lacs-project#426 stripped "; Ubuntu only" from descriptions. Hand-
merging a generated file is how it drifts from its source, so it was regenerated
from the merged catalogue:

    $ UPDATE_ACTION_REFERENCE=1 cargo test -p sysknife-daemon --test action_reference_doc
    test action_reference_doc_is_current ... ok
    $ cargo test -p sysknife-daemon --test action_reference_doc
    test action_reference_doc_is_current ... ok

The result carries both changes: 12 action-steps flatpak commands, zero
`runuser -u testuser`, and the stripped descriptions.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Six Ufw actions still say 'Ubuntu only' in a catalogue Debian now receives

2 participants