The unattended flag implies nothing: --help and docs/cli.md both promise --yes, --max-risk high and --non-interactive

[vladimirrott]

--help and docs/cli.md both say --dangerously-skip-approval implies three flags. The binary wires none of them.

apps/sysknife-cli/src/cli.rs:63-68:

    /// Execute HIGH-risk steps with no human confirmation.
    ///
    /// Requires `SYSKNIFE_I_ACCEPT_UNATTENDED_ROOT=1` in the environment as
    /// well; the flag on its own refuses to run. Implies `--yes`,
    /// `--max-risk high` and `--non-interactive` unless you set a lower
    /// `--max-risk`, which still wins.

apps/sysknife-cli/src/main.rs:26 parses and never mutates the result. build_run_opts at :183 copies the flags across one for one:

        yes: cli.yes,
        max_risk: cli.max_risk.map(crate::approval::MaxRisk::from),
        non_interactive: cli.non_interactive,

and ApprovalPolicy::effective_auto_ceiling at apps/sysknife-cli/src/approval.rs:133 leaves before the ceiling is computed:

        if !self.yes {
            return None;
        }

No default_value_if on yes, max_risk or non_interactive, and no assignment to any of them anywhere in the binary.

The unit test at apps/sysknife-cli/src/approval.rs:251 assumes the wiring lives somewhere it does not:

    fn the_override_is_inert_without_yes() {
        // The flag implies --yes at the CLI layer. If that wiring is ever
        // dropped, the policy must not auto-approve on the override alone.

Measured

Two throwaway tests appended to apps/sysknife-cli/src/main.rs in a scratch clone of a3d17ad, run in a container, then removed. git status --porcelain | wc -l was 0 afterwards.

The first parses ["sysknife", "--dangerously-skip-approval", "check disk usage"] and asserts cli.yes is false, cli.max_risk is None, cli.non_interactive is false. The second sets SYSKNIFE_I_ACCEPT_UNATTENDED_ROOT=1, builds the real RunOpts through build_run_opts, and asserts effective_auto_ceiling() is None and decide_step(&PlanRiskLevel::Low) is RequiresPrompt.

The podman line below carried an absolute path to a cargo target cache on my own
machine when I filed this. I have replaced it with $CARGO_TARGET_CACHE; point that
at any writable directory, or drop both -v .../ct and CARGO_TARGET_DIR and let
cargo build into the checkout. Nothing else about the run changed.

$ podman run --rm --network=none -v "$PWD:/repo:z" -v "$CARGO_TARGET_CACHE:/ct:z" -v "$HOME/.cargo:/cargo:O" -w /repo -e CARGO_HOME=/cargo -e CARGO_TARGET_DIR=/ct/rv436 -e CARGO_NET_OFFLINE=true -e HOME=/tmp docker.io/library/rust:1-slim cargo test -p sysknife-cli --bins --offline maintainer_probe_336
running 2 tests
test maintainer_probe_336::probe_parsed_flags ... ok
test maintainer_probe_336::probe_decision_with_both_keys ... ok

test result: ok. 2 passed; 0 failed; 0 ignored; 0 measured; 268 filtered out; finished in 0.00s

So SYSKNIFE_I_ACCEPT_UNATTENDED_ROOT=1 sysknife --dangerously-skip-approval "check disk usage" prompts for every step, LOW included. An operator following the Unattended mode section into a cron job gets a run that blocks on a prompt instead of one that executes, or gets nothing at all if stdin is closed.

Which side moves

The documentation, not the binary.

A flag that silently switches on three others is the opposite of the deliberateness the rest of this design insists on: the two-key rule exists so that neither a flag left in a script nor a variable left in a profile is enough on its own, and --yes typed explicitly is a third act of intent worth keeping. The current behaviour is also the safe one, which is why this survived unnoticed.

So --dangerously-skip-approval raises the --yes auto-approval ceiling from MEDIUM to HIGH and does nothing else, and every place that describes it says that.

Scope

• apps/sysknife-cli/src/cli.rs:63-68, the doc comment that becomes --help.
• apps/sysknife-cli/src/approval.rs:251-253, the comment inside the_override_is_inert_without_yes. The assertion is correct and describes the binary; only its comment claims otherwise. Rename it if a better name suggests itself, and keep the test.
• docs/cli.md, the Global flags row and the "What it turns off" bullet. docs: align CLI unattended mode contract #436 is open against that file and will carry these two.
• A test that pins the decision rather than the parse: with both keys and no --yes, decide_step on a LOW step is RequiresPrompt. the_override_is_inert_without_yes already proves this at the policy layer; what is missing is the same assertion built from parsed CLI flags through build_run_opts, so a future default_value_if cannot change the contract without turning a test red.

Both prose figures and the test baseline move with any test you add: UPDATE_TEST_BASELINE=1 scripts/test_baseline.sh writes tests/evidence/workspace-tests.json, and the {count} Rust tests literal in README.md, docs/introduction.md and docs/distro-support.md moves in the same commit.

Found while reviewing #436 against #336.

Tests first

Break what the guard protects. Build RunOpts through build_run_opts from
parsed CLI flags with --dangerously-skip-approval and no --yes, then assert
decide_step(&PlanRiskLevel::Low) is RequiresPrompt. That pins the contract at
the layer the documentation describes, which is the layer nothing currently
tests. the_override_is_inert_without_yes proves the same thing one level down,
at the policy, so a future default_value_if on yes could change the user-
visible behaviour with every test still green.

Break the guard's own input. Assert the parse selected the flag. A test that
builds RunOpts from a Cli whose flag never parsed passes for the wrong
reason, and renaming the long flag would not turn it red. Assert
cli.dangerously_skip_approval is true in the same test that asserts the
decision, so the two cannot drift apart.

Fix the comment inside the_override_is_inert_without_yes while you are there.
The assertion is right; only its comment claims wiring that does not exist.

Difficulty

easy. Three files carrying prose and one new test. No VM, no daemon, no
provider credentials. The reading is the work: confirm for yourself that the
binary wires none of the three before you rewrite the sentence that says it does.

Getting started

CONTRIBUTING.md
has the build and test commands. cargo test -p sysknife-cli --bins is the
suite for this one.

Both prose figures and the test baseline move with any test you add:
UPDATE_TEST_BASELINE=1 scripts/test_baseline.sh writes
tests/evidence/workspace-tests.json, and the {count} Rust tests literal in
README.md, docs/introduction.md and docs/distro-support.md moves in the
same commit. It is 1,861 today.

No CLA and no copyright waiver. The project is MIT.

[atanishka308]

I would like to work on this issue. So, kindly assign it to me.

[vladimirrott]

@atanishka308 #440 is yours. I have assigned you and applied claimed, so it shows on your dashboard and other contributors will stay off it.

It is a good match for what you asked for. The binary and its own documentation disagree about one flag: --help and docs/cli.md both say --dangerously-skip-approval implies --yes, --max-risk high and --non-interactive, and apps/sysknife-cli/src/main.rs wires none of the three. The decision in the body is that the documentation moves and the binary stays, so most of this is editing two prose blocks and one stale comment to match the code that is already there.

Two things worth knowing before you start:

docs/cli.md has an open pull request against it, #436 by @armutlutost, covering the same two rows. Leave that file to them and say so in your PR description; the Rust side and the test are the part nobody else is touching.

The test in the scope list is the piece with real teeth. the_override_is_inert_without_yes already proves the policy layer refuses to auto-approve, and what is missing is the same assertion built from parsed CLI flags through build_run_opts, so that a future default_value_if on yes cannot change the contract while every test stays green. Adding one moves tests/evidence/workspace-tests.json and the {count} Rust tests literal in README.md, docs/introduction.md and docs/distro-support.md; UPDATE_TEST_BASELINE=1 scripts/test_baseline.sh writes the first and you edit the other three in the same commit.

No deadline. Ask here if any of it reads ambiguously and I will answer on the thread rather than in a review three days later.

[vladimirrott]

Edited the body. The podman line quoted an absolute path to a cargo target cache on my own machine, which is no use to you and had no business in a public issue. It reads $CARGO_TARGET_CACHE now: point that at any writable directory, or drop it along with -e CARGO_TARGET_DIR and let cargo build into the checkout. The run is otherwise the one I described.

While I was in there I re-checked every reference in the body against b2c823e6, because v0.16.0 landed after I filed this and I did not want you working from line numbers that had moved. All four still land:

$ git rev-parse --short=8 HEAD
b2c823e6
$ sed -n '63,66p' apps/sysknife-cli/src/cli.rs
    /// Execute HIGH-risk steps with no human confirmation.
    ///
    /// Requires `SYSKNIFE_I_ACCEPT_UNATTENDED_ROOT=1` in the environment as
    /// well; the flag on its own refuses to run. Implies `--yes`,
$ sed -n '186,188p' apps/sysknife-cli/src/main.rs
        yes: cli.yes,
        max_risk: cli.max_risk.map(crate::approval::MaxRisk::from),
        non_interactive: cli.non_interactive,
$ sed -n '132,134p' apps/sysknife-cli/src/approval.rs
    pub fn effective_auto_ceiling(&self) -> Option<MaxRisk> {
        if !self.yes {
            return None;
$ sed -n '251,253p' apps/sysknife-cli/src/approval.rs
    fn the_override_is_inert_without_yes() {
        // The flag implies --yes at the CLI layer. If that wiring is ever
        // dropped, the policy must not auto-approve on the override alone.
$ grep -rn 'default_value_if' apps/sysknife-cli/src/ ; echo "rc=$?"
rc=1

So the binary still wires none of the three, and the documentation is still the side that moves.

[vladimirrott]

@atanishka308 added Tests first, Difficulty and Getting started to the body, on top of the edit I described above. No claim changed and nothing about the work moved.

The part worth reading is the second direction under Tests first. A test that builds RunOpts from a Cli whose flag never parsed passes for the wrong reason and would survive a rename of the long flag, so assert cli.dangerously_skip_approval is true in the same test that asserts decide_step returns RequiresPrompt.

The workspace baseline is 1,861 today, and it moves with any test you add:

$ cat tests/evidence/workspace-tests.json
{
  "commands": {
    "frontend_tests": "vitest run (apps/sysknife-shell)",
    "tests": "cargo nextest run --workspace --locked"
  },
  "frontend_tests": 72,
  "tests": 1861,
  "version": 2
}