From 460427f0eaf6bd4c72ff41a3823b0eb3ad342bc4 Mon Sep 17 00:00:00 2001 From: Sinan Eldem Date: Wed, 26 Aug 2026 12:17:16 +0300 Subject: [PATCH 1/2] docs(admin): state how the account and two-factor failures are classified MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every other workflow section in this reference says how its failures map onto exit codes; this one only mentioned the usage error for a prompt it could not offer. The table fills that in, and two notes cover the parts a script author would otherwise have to discover. The lockout is worth spelling out. It arrives as the network code, which normally means "retry", and it is retryable — but only after the delay the server reports, because the code comes from `SlowDown`. A script that retries immediately is simply refused again. A failed `--output-file` write is also called out as 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. Two rows are pinned by tests rather than left to drift: that a lockout is classified as retryable, and that an absent route is a not-found rather than the unsupported code an up-to-date server sends when at-rest protection is unconfigured. The enrollment test that covered the latter was named for a route that does not exist, which is a different failure; it now says what it checks. --- crates/cli/tests/admin_account.rs | 66 ++++++++++++++++++++++++++++++- docs/reference/rc/admin.md | 25 ++++++++++++ 2 files changed, 90 insertions(+), 1 deletion(-) diff --git a/crates/cli/tests/admin_account.rs b/crates/cli/tests/admin_account.rs index 6933daa..ea2e469 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 1810f9b..0954dc8 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 From fa919745fefd86643e84114e679a90b7f7ebcee3 Mon Sep 17 00:00:00 2001 From: Sinan Eldem Date: Wed, 26 Aug 2026 12:17:16 +0300 Subject: [PATCH 2/2] ci: require the BREAKING marker for reference docs only when lines change MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The protected-files check fires on any diff under `docs/reference/rc/`, including a pure addition. Documenting a new command is an addition, and `AGENTS.md` asks for exactly that as step 2 of the Breaking Change process — so the check made every additive PR assert in its description that it was breaking, which is both untrue and the kind of marker that stops meaning anything once it is routine. The reference paths now need the marker only when existing lines move: a rewritten sentence, a removed flag, a deleted page. Rename detection is off on purpose, so moving a page counts as a rewrite — where a reader or a link lands does change. The schema, exit-code and config files keep the old rule. A new exit code or config field extends a contract even when nothing existing moves, so an addition there is exactly what the marker is for. Also collects failures instead of exiting on the first, so a PR touching two protected paths learns about both in one run. --- .github/workflows/ci.yml | 78 +++++++++++++++++++++++++++++++++------- 1 file changed, 66 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cd90135..b1f7333 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: