Skip to content

docs: align CLI unattended mode contract - #436

Open
armutlutost wants to merge 1 commit into
lacs-project:mainfrom
armutlutost:docs/336-cli-unattended-contract
Open

armutlutost wants to merge 1 commit into
lacs-project:mainfrom
armutlutost:docs/336-cli-unattended-contract

Conversation

@armutlutost

Copy link
Copy Markdown

Summary

Align the CLI documentation with the actual unattended-mode behavior and clarify how audit commands and logging work.

Related Issue

Closes #336

Validation

  • Tests added or updated
  • Documentation updated if behavior changed
  • Security impact considered
  • Trust boundary preserved (daemon remains the only privileged executor)
  • CI passes

Notes for Reviewers

  • Document that --dangerously-skip-approval implies --yes, --max-risk high, and --non-interactive.
  • Explain that its consent check applies to every subcommand.
  • Document SYSKNIFE_DATABASE_PATH and clarify that audit commands do not use SYSKNIFE_SOCKET.
  • Clarify that --log-to captures stdout only and show separate stderr capture.
  • npx --yes markdownlint-cli2 docs/cli.md passes.

@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 382ad656f984252264d9a0b787a7112ed8b8558a.

Three of the four gaps in #336 are closed correctly here, and I checked each against the source rather than against the other document. The SYSKNIFE_SOCKET sentence is the one I would have got wrong: run_audit_export never takes a socket at all, it calls sysknife_core::resolve_audit_store() and reads a file, so "audit commands read a local database directly and do not use this socket" is exactly right. SYSKNIFE_DATABASE_PATH lands in the table that was missing it (crates/sysknife-core/src/lib.rs:44 and :150 are where it resolves). The consent-scope paragraph matches apps/sysknife-cli/src/main.rs:112, which checks both keys before the match on the subcommand, so doctor refuses for the same reason a plan does. The banner is on stderr (apps/sysknife-cli/src/unattended.rs:103 and :111, both eprintln!), and --log-to is stdout-only (apps/sysknife-cli/src/runner.rs:655: "Print line to stderr only. Not teed to the log file").

maintainer screen returns INERT for this diff, so there was nothing to sandbox.

Blocking: the one row that moved toward --help moved toward the wrong copy

#336 said "whichever is right, the other has to move". I wrote that line, and I did not check which one was right. Neither is. The binary does not imply those three flags, and copying the --help sentence into docs/cli.md gives an operator two documents agreeing on something the code does not do.

Cli::parse() runs at apps/sysknife-cli/src/main.rs:26 and nothing mutates the result before dispatch. 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 returns before it reaches the ceiling at all:

        if !self.yes {
            return None;
        }

There is 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 is written on the assumption that the wiring exists somewhere else:

    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.

It does not exist. I added two throwaway tests to main.rs in a scratch clone of your head, ran them in a container, and removed them again:

$ 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

The first asserts that after Cli::parse_from(["sysknife", "--dangerously-skip-approval", "check disk usage"]), cli.yes is false, cli.max_risk is None and cli.non_interactive is false. The second sets the consent variable, builds the real RunOpts through build_run_opts, and asserts that effective_auto_ceiling() is None and that a LOW step decides RequiresPrompt. Both pass.

So SYSKNIFE_I_ACCEPT_UNATTENDED_ROOT=1 sysknife --dangerously-skip-approval "check disk usage" prompts a human for every step, including the LOW ones, which is the opposite of what both documents promise. It fails in the safe direction, which is why nobody has hit it, and it is still wrong in the place an operator reads before writing a cron job.

The fix here is one row and one bullet, describing what the binary does today:

Raises the --yes auto-approval ceiling from MEDIUM to HIGH. It does not imply --yes, --max-risk or --non-interactive; pass those yourself. Refuses to run unless SYSKNIFE_I_ACCEPT_UNATTENDED_ROOT=1 is also set.

I have filed the --help text and the test comment as #440, since that is a Rust change and not yours to carry inside a docs PR. It also records the decision, which is that the documentation moves and the binary stays as it is: a flag that switches on three others by itself is the opposite of the deliberateness the two-key rule exists for. Take #440 if you want it; say so and it is yours.

Not blocking

docs/cli.md sits outside both the claim screen and the markdownlint list:

$ grep -n 'cli.md' scripts/check_evidence_claims.py .github/workflows/ci.yml; echo "grep rc=$?"
grep rc=1
``` The internal link check does cover it. That is part of why this file drifted, and it is my gap to close, not yours.

## Your checks are waiting on me

$ gh pr checks 436 --repo lacs-project/sysknife
no checks reported on the 'docs/336-cli-unattended-contract' branch


GitHub holds a first-time contributor's workflow runs until somebody with write access presses "Approve and run", and I keep that manual. Nothing is wrong with your branch.

Push the corrected row and I will re-read it the same day. The other three items are done and I am not asking you to touch them again.

@vladimirrott

Copy link
Copy Markdown
Member

Your checks are running now. They had been held at action_required since you opened this, which is GitHub's pause on a first contribution until someone with write access releases the run. That was mine to press and I had not.

The review item from earlier is still the one thing outstanding, and it is one row in docs/cli.md. Take your time with it.

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.

docs/cli.md and the binary disagree on the unattended flag, and omit the variable that controls audit commands

2 participants