diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cd901358..b1f73335 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -101,28 +101,82 @@ jobs: env: PR_BODY: ${{ github.event.pull_request.body }} run: | + # Any change to these needs the marker: a new exit code, a new config + # field or a new schema property extends a contract even when nothing + # existing moves. PROTECTED_FILES=( - "docs/reference/rc/" "schemas/output_v1.json" "crates/cli/src/exit_code.rs" "crates/core/src/config.rs" ) - CHANGED_FILES=$(git diff --name-only origin/${{ github.base_ref }}...HEAD) + # The command reference is a contract too, but adding a section to it + # is how a new command gets documented โ€” required by AGENTS.md ยง 3 of + # the Breaking Change process, in fact. Demanding the marker for that + # makes every additive PR claim to be breaking, so these paths need it + # only when existing lines move: a rewritten sentence, a removed flag, + # a deleted or renamed page. + ADDITIVE_OK_FILES=( + "docs/reference/rc/" + ) + + BASE="origin/${{ github.base_ref }}" + CHANGED_FILES=$(git diff --name-only "$BASE"...HEAD) + + # Paths under a protected prefix, one per line. + matched_paths() { + local protected_path="${1%/}" + printf '%s\n' "$CHANGED_FILES" \ + | grep -E "^$(printf '%s' "$protected_path" | sed 's/[.[\*^$]/\\&/g')(/|$)" || true + } + + require_marker() { + local subject="$1" + echo "::warning::Protected file modified: $subject" + echo "This change requires the Breaking Change process. See AGENTS.md." + if ! grep -q "BREAKING" <<< "$PR_BODY"; then + echo "::error::Protected file $subject modified without BREAKING marker in PR description" + return 1 + fi + } + + status=0 for file in "${PROTECTED_FILES[@]}"; do - protected_path="${file%/}" - if echo "$CHANGED_FILES" | grep -Fxq "$protected_path" || echo "$CHANGED_FILES" | grep -q "^$protected_path/"; then - echo "::warning::Protected file modified: $file" - echo "This change requires the Breaking Change process. See AGENTS.md." - - # Check if PR body contains BREAKING marker - if ! grep -q "BREAKING" <<< "$PR_BODY"; then - echo "::error::Protected file $file modified without BREAKING marker in PR description" - exit 1 - fi + if [ -n "$(matched_paths "$file")" ]; then + require_marker "$file" || status=1 fi done + + for file in "${ADDITIVE_OK_FILES[@]}"; do + paths=$(matched_paths "$file") + [ -n "$paths" ] || continue + + # Rename detection off on purpose: moving a reference page changes + # where readers and links land, so it should count as a rewrite + # rather than as a no-op. + removed=0 + count=0 + while IFS= read -r path; do + [ -n "$path" ] || continue + count=$((count + 1)) + deleted=$( + git diff --numstat --no-renames "$BASE"...HEAD -- "$path" \ + | awk '{ total += $2 } END { print total + 0 }' + ) + removed=$((removed + deleted)) + done <<< "$paths" + + if [ "$removed" -gt 0 ]; then + require_marker "$file ($removed line(s) changed or removed)" || status=1 + else + echo "$file changed by addition only ($count file(s)); no marker required" + fi + done + + if [ "$status" -ne 0 ]; then + exit 1 + fi echo "Protected files check passed" msrv: diff --git a/crates/cli/tests/admin_account.rs b/crates/cli/tests/admin_account.rs index 6933daad..ea2e469f 100644 --- a/crates/cli/tests/admin_account.rs +++ b/crates/cli/tests/admin_account.rs @@ -195,7 +195,9 @@ fn mfa_status_succeeds_against_a_server_that_answers() { } #[test] -fn mfa_enroll_reports_unsupported_when_the_server_has_no_such_route() { +fn mfa_enroll_reports_unsupported_when_at_rest_protection_is_missing() { + // 501, which the server sends when `RUSTFS_IAM_MASTER_KEY` is unset. Not to + // be confused with an absent route, which is a 404 and a different code. let config_dir = tempfile::tempdir().expect("create config dir"); let (endpoint, receiver, handle) = start_admin_response_test_server( "501 Not Implemented", @@ -540,3 +542,65 @@ fn user_mfa_status_reports_auth_error_when_the_server_refuses() { .expect("captured admin request"); handle.join().expect("admin test server finished"); } + +// --------------------------------------------------------------------------- +// The exit-code table in docs/reference/rc/admin.md +// --------------------------------------------------------------------------- + +/// The reference documents how each failure is classified, and that file is a +/// protected contract. These pin the two rows a reader is most likely to build +/// retry logic around, so the table cannot drift away from the code silently. +#[test] +fn a_lockout_is_reported_as_retryable_rather_than_a_refusal() { + // The server answers `SlowDown` once the attempt limit is reached. That maps + // to the network code, which normally means "retry" โ€” the reference says why + // it means "retry later" here, and this holds it to that class. + let config_dir = tempfile::tempdir().expect("create config dir"); + let (endpoint, receiver, handle) = start_admin_response_test_server( + "503 Service Unavailable", + "application/json", + r#"{"Code":"SlowDown","Message":"too many attempts; try again in 900 seconds"}"# + .to_string(), + ); + + let output = rc() + .args([ + "--json", "admin", "account", "mfa", "activate", "myalias", "--code", "123456", + ]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_eq!(output.status.code(), Some(3), "expected NetworkError"); + receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + handle.join().expect("admin test server finished"); +} + +#[test] +fn an_absent_route_is_reported_as_not_found() { + // A server predating these endpoints. Distinct from the 501 an up-to-date + // server sends when at-rest protection is unconfigured, which is the row + // above it in the table. + let config_dir = tempfile::tempdir().expect("create config dir"); + let (endpoint, receiver, handle) = start_admin_response_test_server( + "404 Not Found", + "application/json", + r#"{"Code":"NoSuchKey","Message":"unknown route"}"#.to_string(), + ); + + let output = rc() + .args(["--json", "admin", "account", "mfa", "status", "myalias"]) + .env("RC_CONFIG_DIR", config_dir.path()) + .env("RC_HOST_myalias", rc_host_alias(&endpoint)) + .output() + .expect("run rc command"); + + assert_eq!(output.status.code(), Some(5), "expected NotFound"); + receiver + .recv_timeout(Duration::from_secs(5)) + .expect("captured admin request"); + handle.join().expect("admin test server finished"); +} diff --git a/docs/reference/rc/admin.md b/docs/reference/rc/admin.md index 1810f9bf..0954dc83 100644 --- a/docs/reference/rc/admin.md +++ b/docs/reference/rc/admin.md @@ -191,6 +191,31 @@ asks for confirmation; `--yes` is required in `--json` mode or when stdin is not a terminal. The account is left protected by its password alone until the user enrols again. +### Exit codes + +Every other workflow in this reference states how its failures are classified, so +these do too. The account and two-factor commands map the server's answer onto +the standard codes: + +| Condition | Code | +|---|---| +| Wrong verification code, recovery code, or password | authentication (4) | +| Locked out after repeated wrong codes | network (3) | +| At-rest protection not configured on the server | unsupported (7) | +| Route absent, on a server predating these endpoints | not found (5) | +| Recovery-code output path already occupied | conflict (6) | +| Missing, conflicting, or unprompted flags | usage (2) | +| Already enabled, not enabled, or no pending enrollment | general (1) | + +The lockout deserves a note, because network (3) usually means "retry". It is +retryable here too, but only after the delay the server reports โ€” the code comes +from `SlowDown`, which is the S3 vocabulary's closest analogue to a rate limit. +A script that retries immediately will simply be refused again. + +A failed `--output-file` write is the one case where a non-zero exit does not mean +nothing happened: the server has already issued the set, so the codes are printed +and the exit code reports only that the file was not written. + ### Root identities A root identity provisioned from `RUSTFS_ACCESS_KEY` cannot have its password or