lacs-project / lacs-project/sysknife
The unattended flag implies nothing: --help and docs/cli.md both promise --yes, --max-risk high and --non-interactive
- Dominant language
- Rust
- Stars
- 12
- Forks
- 19
- Avg merge
- 18h 57m
- Merged PRs (30d)
- 116
Description
`--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`.
```
$ podman run --rm --network=none -v "$PWD:/repo:z" -v /home/entropia/.local/state/sysknife-maint/ctargets:/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. #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.
Contributor guide
Research direction
Read the flag documentation in apps/sysknife-cli/src/cli.rs, parsing and build_run_opts in src/main.rs, and effective_auto_ceiling in src/approval.rs. Run the focused approval tests, then add coverage for parsed flags reaching a LOW-step decision and update docs/cli.md and the named baseline count files. Done means the flag raises the approval ceiling as documented without implying --yes, --max-risk high, or --non-interactive.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- cli, documentation, security, testing
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 72/100