diff --git a/.kiro/specs/xce-test-cli-ux/tasks.md b/.kiro/specs/xce-test-cli-ux/tasks.md index e390311d..311b9cc3 100644 --- a/.kiro/specs/xce-test-cli-ux/tasks.md +++ b/.kiro/specs/xce-test-cli-ux/tasks.md @@ -114,7 +114,7 @@ - _Boundary: Interruptible Call Adapter_ - _Depends: 1.4, 1.5, 3.1_ -- [ ] 3.3 Wire the five destructive commands to the confirmation gate +- [x] 3.3 Wire the five destructive commands to the confirmation gate - Gate the job-kill, shutdown, queue-close/inactivate, host-close, and configuration-upgrade actions behind the confirmation gate built in 2.1, using each command's now-correctly-parsed flags to decide whether a given invocation counts as destructive - Add the bypass flag to each of these commands via the shared flag-descriptor table - Observable completion: each of the five destructive actions prompts interactively by default, proceeds without prompting when the bypass flag or the session toggle is used, and proceeds without prompting when input is not interactive; separately, confirm that a reversal invocation (queue-active/open, host-open) and the fake-worker job-lifecycle commands never prompt at all, since they are excluded from the destructive set @@ -178,6 +178,7 @@ ## Implementation Notes - `src/tools/xce-build.sh controller asan xce.build --incremental` writes its build dir at `src/xce.build/` (relative to the script's own location), not a repo-root `xce.build/` — meson logs are at `src/xce.build/meson-logs/testlog.txt`. (task 1.1) - New Unity test files need two wiring points, not just meson.build's source list: also register an `extern void test_(void);` + `TEST(test_);` in `src/controller/testsuite/controller_test.c`, mirroring the existing `test_rcfile` registration, or the suite silently never runs. (task 1.1) +- Confirmed (review traced it concretely): a declined destructive command inside a `source`d file does NOT trip `set -e`'s abort-on-failure, since decline maps to `return 0` per confirm.h's own documented contract, not `execute_line`'s `rc<0` failure check. This only matters when a human is actively present declining (never a blind/unattended failure mode), and any later destructive line re-prompts independently — but task 4.3 should add an explicit `set -e` + `source ` case to prove/document this interaction, since it's currently untested. (task 3.3) - **Safety-critical finding**: wrapping RPC calls in an abandonable worker thread means `leave_ctx()` (called by `up`/`quit`) can now run while a previously-interrupted command's worker is still using the connection — before this task the REPL was fully synchronous so this UAF couldn't exist. Fixed via `xcetest_wait_connection_idle(conn_key)` (interruptcall.c), which blocks on the SAME per-connection semaphore the adapter already uses, then immediately releases it, proving no worker is active before teardown proceeds. Call it before any future connection-teardown path, not just `leave_ctx()`, if one is added later. A timing-dependent black-box test alone could NOT reliably prove this race (ran clean 20/20 even with the fix disabled) — the reliable proof came from a white-box test directly exercising the wait function against a real abandoned worker. When a fix's failure mode is a sub-millisecond race, don't trust a black-box test's clean run as RED-phase evidence; get a deterministic white-box reproduction instead. (task 3.2, bug-1795) - All 17 flag-accepting commands migrated to `flagspec_t`+`getopt_long` (review verified essentially every command line-by-line via git diff context, not just a sample — zero regressions to any existing short flag's letter/value-format/default). `cmd_walcompare`'s migration needs a leading `+` in its optstring (`getopt_long`'s "stop at first non-option") to preserve its pre-migration "flags only before positionals" behavior — without it, argv permuting would change which tokens are treated as flags vs. member paths. Only 7 of the 17 migrated commands got dedicated black-box tests (`qctrl`, `jobinfo`, `jobkill`, `hostctrl`, `setpend`, `resupdate`, `walcompare`); the other 10 (`hello`, `jobrun`, `jobsub`, `resval`, `jobckpt`, `runtasks`, `jobstart`, `jobfinish`, `jobfail`, `join_worker_parse_attrs`) have no valid-flag regression test at all beyond this review's manual line-by-line diff check — worth a follow-up smoke test per command if this area gets touched again. (task 3.1) - `cmd_join_worker`'s sibling fields `maxswap`/`maxtmp` have the exact same stale-MB-scale bug as bug-1557's `maxmem` (1024/2048 bytes instead of KB/MB-appropriate values) but were deliberately left unfixed here: `USAGE_JOIN_WORKER` currently documents these exact stale values accurately, so fixing only the code would create a new code/docs mismatch outside this task's mandate. Worth a coordinated follow-up bug fixing both fields' defaults and their help text together. A new `test_join_worker_sibling_defaults_unchanged` test locks in today's values so that follow-up isn't confused with this task's maxmem-specific assertions. (task 2.6) diff --git a/src/controller/testsuite/test.c b/src/controller/testsuite/test.c index 4e96ca3e..3b825150 100644 --- a/src/controller/testsuite/test.c +++ b/src/controller/testsuite/test.c @@ -361,6 +361,7 @@ static const char USAGE_SET[] = "set\n" "set \n" "set -e | +e | continue-on-error\n" + "set -y | +y\n" " (no args) list all settings in the current context\n" " (current value + default)\n" " set setting (e.g. masters, secret,\n" @@ -369,6 +370,12 @@ static const char USAGE_SET[] = " command (errexit)\n" " +e | continue-on-error keep the read loop running after a\n" " failing command\n" + " -y skip the confirmation prompt before a\n" + " destructive command (jobkill, shutdown,\n" + " qctrl -i/-c, hostctrl -c, upgradeconfig)\n" + " regardless of TTY state, until +y\n" + " +y require confirmation before those\n" + " commands again (default)\n" "example: set masters ctrl1=localhost:9991,ctrl2=localhost:9992\n"; static const char USAGE_JOIN_WORKER[] = @@ -1897,10 +1904,59 @@ static int shutdown_thunk(void* ctxp) return rc; } +/* xce-test-cli-ux task 3.3, Requirement 9.3: confirmation bypass. shutdown + * had no flags at all before this task (task 3.1's getopt migration + * explicitly left it out of scope for exactly that reason) -- this is the + * FIRST flagspec_t table it gets, added solely to carry --yes. Same + * short/long spelling as the other four destructive commands + * (Requirement 6.2). */ +static const flagspec_t shutdown_flags[] = { + { 'y', "yes", "yes", no_argument, 0, NULL }, +}; +#define SHUTDOWN_NFLAGS (sizeof(shutdown_flags) / sizeof(shutdown_flags[0])) + int cmd_shutdown(int argc, char* argv[]) { int rc; - shutdown_call_t* call = (shutdown_call_t*)calloc(1, sizeof(*call)); + int opt; + int yes = 0; + struct option long_opts[SHUTDOWN_NFLAGS + 1]; + char optstring[16]; + shutdown_call_t* call; + + optind = 1; + opterr = 0; + + if (flagspec_build_opts(shutdown_flags, SHUTDOWN_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: shutdown: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { + switch (opt) { + case 'y': + yes = 1; + break; + case '?': + return flagspec_reject("shutdown", shutdown_flags, + SHUTDOWN_NFLAGS, argv[optind - 1], + "shutdown"); + } + } + + /* xce-test-cli-ux task 3.3, Requirement 9.5: shutdown is unconditionally + * destructive. See cmd_jobkill's comment above for why this call site + * only needs to check its own --yes flag. */ + if (!yes && !confirm_destructive("shut down the xce controller")) { + fprintf(stderr, "shutdown: not run (declined)\n"); + return 0; + } + + call = (shutdown_call_t*)calloc(1, sizeof(*call)); if (call == NULL) { return -ENOMEM; @@ -3529,6 +3585,12 @@ int cmd_resval(int argc, char* argv[]) static const flagspec_t jobkill_flags[] = { { 's', "signal", "signal", required_argument, 0, "SIGTERM" }, { 'j', "jobid", "jobid", required_argument, 1, NULL }, + /* xce-test-cli-ux task 3.3, Requirement 9.3: per-command confirmation + * bypass -- jobkill is unconditionally destructive (Requirement 9.5), so + * this flag's only effect is to skip the confirm_destructive() prompt + * below. Same short/long spelling used across all five destructive + * commands (Requirement 6.2 cross-command consistency). */ + { 'y', "yes", "yes", no_argument, 0, NULL }, }; #define JOBKILL_NFLAGS (sizeof(jobkill_flags) / sizeof(jobkill_flags[0])) @@ -3554,6 +3616,7 @@ int cmd_jobkill(int argc, char* argv[]) uint64_t jobid = 0; int opt; int rc; + int yes = 0; xce_jobctrl_t jobctrl; xce_signal_t siginfo; char jobidStr[64]; @@ -3581,6 +3644,9 @@ int cmd_jobkill(int argc, char* argv[]) case 'j': parse_jobid(&jobid, optarg); break; + case 'y': + yes = 1; + break; case '?': return flagspec_reject("jobkill", jobkill_flags, JOBKILL_NFLAGS, argv[optind - 1], @@ -3593,6 +3659,28 @@ int cmd_jobkill(int argc, char* argv[]) return -1; } + /* xce-test-cli-ux task 3.3, Requirement 9.5: jobkill is unconditionally + * destructive -- no action-dependent check like qctrl/hostctrl below. + * confirm_destructive() already owns the session-toggle and TTY checks + * internally (confirm.h); this call site's only job is to short-circuit + * around it when jobkill's OWN --yes flag was given (confirm_destructive + * has no way to know about a per-command flag). A 0 return means the + * operator declined (or Ctrl-C'd the prompt, Requirement 9.9) -- report + * "not run" (a plain, non-negative return so the session verdict + * machinery in preprocess_and_run treats this as success, not failure -- + * see confirm.h's return-contract note), distinct from a "failed" RPC. */ + fmt_jobid(jobidStr, sizeof(jobidStr) - 1, jobid); + + if (!yes) { + char desc[96]; + + snprintf(desc, sizeof(desc), "kill job %s", jobidStr); + if (!confirm_destructive(desc)) { + fprintf(stderr, "jobkill: not run (declined)\n"); + return 0; + } + } + jobctrl.action = JobControl_u_siginfo; jobctrl.jobid = jobid; jobctrl.u.siginfo = &siginfo; @@ -3624,7 +3712,6 @@ int cmd_jobkill(int argc, char* argv[]) return rc; } - fmt_jobid(jobidStr, sizeof(jobidStr) - 1, jobid); printf("Job %s signaled\n", jobidStr); return 0; @@ -3755,6 +3842,12 @@ static const flagspec_t qctrl_flags[] = { { 'o', "open", "open", no_argument, 0, NULL }, { 'c', "close", "close", no_argument, 0, NULL }, { 'q', "queue", "queue", required_argument, 1, NULL }, + /* xce-test-cli-ux task 3.3, Requirement 9.3: confirmation bypass. + * qctrl is destructive ONLY for -i/--deactivate and -c/--close + * (Requirement 9.5); -a/--activate and -o/--open are reversal/ + * reactivation actions and never prompt. Same short/long spelling as + * the other four destructive commands (Requirement 6.2). */ + { 'y', "yes", "yes", no_argument, 0, NULL }, }; #define QCTRL_NFLAGS (sizeof(qctrl_flags) / sizeof(qctrl_flags[0])) @@ -3786,6 +3879,7 @@ int cmd_qctrl(int argc, char* argv[]) * indeterminate stack memory as the queue action. */ int action = QUEUE_DO_NONE; char* queue = NULL; + int yes = 0; struct option long_opts[QCTRL_NFLAGS + 1]; char optstring[16]; @@ -3818,6 +3912,9 @@ int cmd_qctrl(int argc, char* argv[]) case 'q': queue = strdup(optarg); break; + case 'y': + yes = 1; + break; case '?': FREEUP(queue); return flagspec_reject("qctrl", qctrl_flags, QCTRL_NFLAGS, @@ -3841,6 +3938,23 @@ int cmd_qctrl(int argc, char* argv[]) return -1; } + /* xce-test-cli-ux task 3.3, Requirement 9.5: qctrl is destructive ONLY + * for -i/--deactivate and -c/--close -- -a/--activate and -o/--open are + * reversal/reactivation actions and never prompt, regardless of short + * or long spelling. See cmd_jobkill's comment above for why this call + * site only needs to check its own --yes flag. */ + if (((action == QUEUE_INACTIVE) || (action == QUEUE_CLOSE)) && !yes) { + char desc[160]; + const char* verb = (action == QUEUE_CLOSE) ? "close" : "deactivate"; + + snprintf(desc, sizeof(desc), "%s queue %s", verb, queue); + if (!confirm_destructive(desc)) { + fprintf(stderr, "qctrl: not run (declined)\n"); + free(queue); + return 0; + } + } + queuectrl.action = action; queuectrl.queue = queue; @@ -3890,6 +4004,11 @@ static const flagspec_t hostctrl_flags[] = { { 'o', "open", "open", no_argument, 0, NULL }, { 'c', "close", "close", no_argument, 0, NULL }, { 'h', "host", "host", required_argument, 1, NULL }, + /* xce-test-cli-ux task 3.3, Requirement 9.3: confirmation bypass. + * hostctrl is destructive ONLY for -c/--close (Requirement 9.5); + * -o/--open is a reactivation action and never prompts. Same short/long + * spelling as the other four destructive commands (Requirement 6.2). */ + { 'y', "yes", "yes", no_argument, 0, NULL }, }; #define HOSTCTRL_NFLAGS (sizeof(hostctrl_flags) / sizeof(hostctrl_flags[0])) @@ -3915,6 +4034,7 @@ int cmd_hostctrl(int argc, char* argv[]) xce_hostctrl_t hostctrl; int action = HOST_DO_NONE; char* hostname = NULL; + int yes = 0; struct option long_opts[HOSTCTRL_NFLAGS + 1]; char optstring[16]; @@ -3941,6 +4061,9 @@ int cmd_hostctrl(int argc, char* argv[]) case 'h': hostname = strdup(optarg); break; + case 'y': + yes = 1; + break; case '?': FREEUP(hostname); return flagspec_reject("hostctrl", hostctrl_flags, @@ -3960,6 +4083,22 @@ int cmd_hostctrl(int argc, char* argv[]) return -1; } + /* xce-test-cli-ux task 3.3, Requirement 9.5: hostctrl is destructive + * ONLY for -c/--close -- -o/--open is a reactivation action and never + * prompts, regardless of short or long spelling. See cmd_jobkill's + * comment above for why this call site only needs to check its own + * --yes flag. */ + if ((action == HOST_CLOSE) && !yes) { + char desc[160]; + + snprintf(desc, sizeof(desc), "close host %s", hostname); + if (!confirm_destructive(desc)) { + fprintf(stderr, "hostctrl: not run (declined)\n"); + free(hostname); + return 0; + } + } + hostctrl.action = action; hostctrl.hostname = hostname; @@ -4029,14 +4168,58 @@ static int upgradeconfig_thunk(void* ctxp) return rc; } +/* xce-test-cli-ux task 3.3, Requirement 9.3: confirmation bypass. + * upgradeconfig had no flags at all before this task (task 3.1's getopt + * migration explicitly left it out of scope for exactly that reason, + * alongside shutdown) -- this is the FIRST flagspec_t table it gets, added + * solely to carry --yes; the path stays a plain positional + * argument (argv[optind] once getopt_long has consumed --yes). Same + * short/long spelling as the other four destructive commands + * (Requirement 6.2). */ +static const flagspec_t upgradeconfig_flags[] = { + { 'y', "yes", "yes", no_argument, 0, NULL }, +}; +#define UPGRADECONFIG_NFLAGS \ + (sizeof(upgradeconfig_flags) / sizeof(upgradeconfig_flags[0])) + int cmd_upgradeconfig(int argc, char* argv[]) { - if (argc != 2) { + int opt; + int yes = 0; + struct option long_opts[UPGRADECONFIG_NFLAGS + 1]; + char optstring[16]; + + optind = 1; + opterr = 0; + + if (flagspec_build_opts(upgradeconfig_flags, UPGRADECONFIG_NFLAGS, + long_opts, optstring, sizeof(optstring)) + < 0) { + fprintf( + stderr, + "xce_test: upgradeconfig: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { + switch (opt) { + case 'y': + yes = 1; + break; + case '?': + return flagspec_reject("upgradeconfig", upgradeconfig_flags, + UPGRADECONFIG_NFLAGS, argv[optind - 1], + "upgradeconfig "); + } + } + + if ((argc - optind) != 1) { fprintf(stderr, "usage: upgradeconfig \n"); return -1; } - const char* path = argv[1]; + const char* path = argv[optind]; FILE* fp = fopen(path, "rb"); if (fp == NULL) { fprintf(stderr, "upgradeconfig: cannot open %s: %s\n", path, @@ -4078,6 +4261,23 @@ int cmd_upgradeconfig(int argc, char* argv[]) uint8_t sha256[SHA256_DIGEST_LENGTH]; SHA256(bytes, (size_t)fsz, sha256); + /* xce-test-cli-ux task 3.3, Requirement 9.5: upgradeconfig is + * unconditionally destructive. See cmd_jobkill's comment above for why + * this call site only needs to check its own --yes flag. Deliberately + * placed after the file has already been read/hashed (a purely local, + * read-only step) and right before the network call itself. */ + if (!yes) { + char desc[PATH_MAX + 64]; + + snprintf(desc, sizeof(desc), + "upgrade the cluster configuration from %s", path); + if (!confirm_destructive(desc)) { + fprintf(stderr, "upgradeconfig: not run (declined)\n"); + free(bytes); + return 0; + } + } + upgradeconfig_call_t* call = (upgradeconfig_call_t*)calloc(1, sizeof(*call)); int rc; @@ -5467,6 +5667,22 @@ int cmd_set(int argc, char* argv[]) return 0; } + /* Confirmation-skip toggle (xce-test-cli-ux task 3.3, Requirement + * 9.6/9.7), same shell-style on/off pair as `-e`/`+e` immediately + * above: `-y` enables skipping (assume yes to every destructive + * command's prompt, regardless of TTY state), `+y` restores the default + * (confirmation required). Wraps confirm_set_skip()/confirm_is_skip_set() + * (confirm.h, task 2.1), which the five destructive commands' own + * confirm_destructive() calls already consult internally. */ + if ((argc == 2) && (strcmp(argv[1], "-y") == 0)) { + confirm_set_skip(1); /* -y: skip destructive-command confirmation */ + return 0; + } + if ((argc == 2) && (strcmp(argv[1], "+y") == 0)) { + confirm_set_skip(0); /* +y: require confirmation again (default) */ + return 0; + } + if (argc != 3) { printf("set \n"); return -EINVAL; diff --git a/src/controller/testsuite/test_test_cli.c b/src/controller/testsuite/test_test_cli.c index bcea7bb3..01e6fd1f 100644 --- a/src/controller/testsuite/test_test_cli.c +++ b/src/controller/testsuite/test_test_cli.c @@ -1897,6 +1897,416 @@ static void test_interrupt_then_immediate_teardown_no_uaf(void) clitest_buf_free(&err); } +/* + * test_qctrl_hostctrl_confirm_gate_destructiveness_determination -- + * xce-test-cli-ux task 3.3, Requirement 9.5: qctrl and hostctrl are the two + * CONDITIONALLY destructive commands -- only -i/-c (qctrl) and -c + * (hostctrl) require confirmation; -a/-o (qctrl) and -o (hostctrl) are + * reversal/reactivation actions and must never prompt. This test proves + * the command-specific determination logic itself (each command's own + * `action == ...` check), not confirm_destructive()'s generic mechanism + * (already covered by test_confirm.c, task 2.1). Declining also proves + * Requirement 9.1/9.2: a prompt appears, and declining reports "not run" + * rather than "failed", without disrupting session responsiveness. + */ +static void test_qctrl_hostctrl_confirm_gate_destructiveness_determination( + void) +{ + char xce_test_path[PATH_MAX]; + char* const argv[] = { "xce_test", "--norc", NULL }; + clitest_session_t sess; + clitest_buf_t out; + clitest_buf_t err; + int exit_code = -999; + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, resolve_xce_test_path(xce_test_path, sizeof(xce_test_path)), + "must be able to resolve the sibling xce_test binary's path"); + + clitest_buf_init(&out); + clitest_buf_init(&err); + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, clitest_spawn(&sess, xce_test_path, argv), + "clitest_spawn must launch xce_test behind a real pty"); + + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "xce>", false, 5000), + "expected the initial 'xce>' interactive prompt on stdout"); + + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "client")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "xce/client>", false, 5000), + "expected to enter the client context"); + + /* qctrl -a (ACTIVE) is a reversal/reactivation action and must never + * prompt (Requirement 9.5). */ + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "qctrl -q normal -a")); + TEST_ASSERT_EQUAL_INT( + 0, clitest_send_line(&sess, "echo marker_qctrl_a_done")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "marker_qctrl_a_done", + false, 5000), + "qctrl -a must run to completion without blocking on a prompt"); + TEST_ASSERT_NULL_MESSAGE( + strstr(err.data, "About to"), + "qctrl -a (ACTIVE) is a reversal action and must never prompt " + "(Requirement 9.5)"); + + /* qctrl -o (OPEN) likewise must never prompt. */ + clitest_buf_free(&err); + clitest_buf_init(&err); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "qctrl -q normal -o")); + TEST_ASSERT_EQUAL_INT( + 0, clitest_send_line(&sess, "echo marker_qctrl_o_done")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "marker_qctrl_o_done", + false, 5000), + "qctrl -o must run to completion without blocking on a prompt"); + TEST_ASSERT_NULL_MESSAGE( + strstr(err.data, "About to"), + "qctrl -o (OPEN) is a reversal action and must never prompt " + "(Requirement 9.5)"); + + /* qctrl -c (CLOSE) IS destructive: must prompt, and declining must + * report "not run" (Requirement 9.1, 9.2, 9.5). */ + clitest_buf_free(&err); + clitest_buf_init(&err); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "qctrl -q normal -c")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "About to close queue normal. Continue?", + true, 5000), + "qctrl -c (CLOSE) must prompt for confirmation (Requirement 9.1, " + "9.5)"); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "n")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "not run", true, 5000), + "declining the prompt must report the command as not run " + "(Requirement 9.2)"); + + /* The session must remain fully responsive in the same context after a + * decline. */ + TEST_ASSERT_EQUAL_INT( + 0, clitest_send_line(&sess, "echo marker_after_qctrl_decline")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "marker_after_qctrl_decline", false, 5000), + "the session must stay responsive after declining a destructive " + "command"); + + /* hostctrl -o (OPEN) is a reactivation action and must never prompt. */ + clitest_buf_free(&err); + clitest_buf_init(&err); + TEST_ASSERT_EQUAL_INT(0, + clitest_send_line(&sess, "hostctrl -h node01 -o")); + TEST_ASSERT_EQUAL_INT( + 0, clitest_send_line(&sess, "echo marker_hostctrl_o_done")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "marker_hostctrl_o_done", + false, 5000), + "hostctrl -o must run to completion without blocking on a prompt"); + TEST_ASSERT_NULL_MESSAGE( + strstr(err.data, "About to"), + "hostctrl -o (OPEN) is a reactivation action and must never prompt " + "(Requirement 9.5)"); + + /* hostctrl -c (CLOSE) IS destructive: must prompt, and declining must + * report "not run". */ + clitest_buf_free(&err); + clitest_buf_init(&err); + TEST_ASSERT_EQUAL_INT(0, + clitest_send_line(&sess, "hostctrl -h node01 -c")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "About to close host node01. Continue?", true, + 5000), + "hostctrl -c (CLOSE) must prompt for confirmation (Requirement 9.1, " + "9.5)"); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "n")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "not run", true, 5000), + "declining the prompt must report the command as not run " + "(Requirement 9.2)"); + + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "quit")); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, clitest_wait_for_exit(&sess, &out, &err, 5000, &exit_code), + "xce_test did not exit within the harness's safety timeout after " + "'quit'"); + + clitest_kill_and_reap(&sess); + clitest_close(&sess); + clitest_buf_free(&out); + clitest_buf_free(&err); +} + +/* + * test_destructive_commands_prompt_by_default_and_yes_bypasses -- + * xce-test-cli-ux task 3.3, Requirement 9.1, 9.2, 9.3, 9.5: jobkill, + * shutdown, and upgradeconfig are UNCONDITIONALLY destructive (no + * qctrl/hostctrl-style action check). This proves the identical + * confirm_destructive() wiring at all three call sites -- each prompts by + * default and declining reports "not run" without ever reaching the + * network call -- plus jobkill's own --yes flag proving the per-command + * bypass (Requirement 9.3) skips the prompt and lets the command proceed. + * One test covers all three rather than duplicating the same mechanism + * five times over (see this task's own brief on proportionate coverage). + */ +static void test_destructive_commands_prompt_by_default_and_yes_bypasses( + void) +{ + char xce_test_path[PATH_MAX]; + char* const argv[] = { "xce_test", "--norc", NULL }; + clitest_session_t sess; + clitest_buf_t out; + clitest_buf_t err; + char fixture_path[320]; + int exit_code = -999; + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, resolve_xce_test_path(xce_test_path, sizeof(xce_test_path)), + "must be able to resolve the sibling xce_test binary's path"); + + clitest_fixture_fresh_dir(); + snprintf(fixture_path, sizeof(fixture_path), "%s/cluster.bin", + clitest_fixture_dir); + clitest_fixture_write_file( + fixture_path, + "not a real capnp blob -- upgradeconfig only needs to read+hash it " + "before the confirmation gate is reached\n"); + + clitest_buf_init(&out); + clitest_buf_init(&err); + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, clitest_spawn(&sess, xce_test_path, argv), + "clitest_spawn must launch xce_test behind a real pty"); + + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "xce>", false, 5000), + "expected the initial 'xce>' interactive prompt on stdout"); + + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "client")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "xce/client>", false, 5000), + "expected to enter the client context"); + + /* jobkill: unconditionally destructive -- prompts by default. + * "1-1-1-1" (not a bare "1") is required: parse_jobid() only accepts + * the dash-delimited cluster-apiserver-timestamp-job encoding + * (REGEX_JOBID, config.h) or its bracketed array-index form + * (REGEX_JOBARRAY_ID) -- a bare decimal never matches either regex and + * silently leaves jobid at 0, which would hit cmd_jobkill's OWN "you + * need to specify jobid with -j" rejection before ever reaching the + * confirmation gate under test here. */ + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "jobkill -j 1-1-1-1")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "About to kill job", true, + 5000), + "jobkill must prompt for confirmation by default (Requirement 9.1, " + "9.5)"); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "n")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "jobkill: not run", true, + 5000), + "declining jobkill's prompt must report it as not run, not failed " + "(Requirement 9.2)"); + + /* jobkill --yes: the per-command bypass flag (Requirement 9.3) must + * skip the prompt entirely and let the command proceed to the RPC + * (which fails fast here since no live controller is running in this + * unit-test environment -- that failure itself is not under test). */ + clitest_buf_free(&err); + clitest_buf_init(&err); + TEST_ASSERT_EQUAL_INT( + 0, clitest_send_line(&sess, "jobkill -j 1-1-1-1 --yes")); + TEST_ASSERT_EQUAL_INT( + 0, clitest_send_line(&sess, "echo marker_jobkill_yes_done")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "marker_jobkill_yes_done", + false, 5000), + "jobkill --yes must run to completion without blocking on a " + "prompt"); + TEST_ASSERT_NULL_MESSAGE( + strstr(err.data, "About to"), + "jobkill --yes must skip the confirmation prompt entirely " + "(Requirement 9.3)"); + TEST_ASSERT_NULL_MESSAGE(strstr(err.data, "not run"), + "jobkill --yes must proceed to the RPC, not " + "report itself as not run"); + + /* shutdown: unconditionally destructive -- prompts by default; + * declining must NOT leave the client context (the context switch is + * gated behind an actually-successful RPC, never behind the decline + * path -- Requirement 9.2). */ + clitest_buf_free(&err); + clitest_buf_init(&err); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "shutdown")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text( + &sess, &out, &err, + "About to shut down the xce controller. Continue?", true, 5000), + "shutdown must prompt for confirmation by default (Requirement 9.1, " + "9.5)"); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "n")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "shutdown: not run", true, + 5000), + "declining shutdown's prompt must report it as not run, not failed " + "(Requirement 9.2)"); + + /* Proves the client context was never left: qctrl only exists in the + * client context's command table, so it being recognized (rather than + * "No such command") proves the session is still there. */ + clitest_buf_free(&err); + clitest_buf_init(&err); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "qctrl -q normal -a")); + TEST_ASSERT_EQUAL_INT( + 0, clitest_send_line(&sess, "echo marker_still_in_client")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "marker_still_in_client", + false, 5000), + "the session must still be in the client context after declining " + "shutdown"); + TEST_ASSERT_NULL_MESSAGE( + strstr(err.data, "No such command"), + "qctrl must still be recognized -- the client context must not " + "have been left by a declined shutdown"); + + /* upgradeconfig: unconditionally destructive -- prompts by default. */ + clitest_buf_free(&err); + clitest_buf_init(&err); + { + char cmd[400]; + + snprintf(cmd, sizeof(cmd), "upgradeconfig %s", fixture_path); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, cmd)); + } + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text( + &sess, &out, &err, + "About to upgrade the cluster configuration from", true, 5000), + "upgradeconfig must prompt for confirmation by default " + "(Requirement 9.1, 9.5)"); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "n")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "upgradeconfig: not run", + true, 5000), + "declining upgradeconfig's prompt must report it as not run, not " + "failed (Requirement 9.2)"); + + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "quit")); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, clitest_wait_for_exit(&sess, &out, &err, 5000, &exit_code), + "xce_test did not exit within the harness's safety timeout after " + "'quit'"); + + clitest_kill_and_reap(&sess); + clitest_close(&sess); + clitest_buf_free(&out); + clitest_buf_free(&err); + clitest_fixture_cleanup_dir(); +} + +/* + * test_confirm_gate_session_toggle_skips_and_restores -- xce-test-cli-ux + * task 3.3, Requirement 9.6/9.7: `set -y` must make every destructive + * command skip its confirmation prompt regardless of TTY state (proven + * here against a genuinely interactive pty session -- the strictest + * case), and `set +y` must restore the default prompting behavior. + */ +static void test_confirm_gate_session_toggle_skips_and_restores(void) +{ + char xce_test_path[PATH_MAX]; + char* const argv[] = { "xce_test", "--norc", NULL }; + clitest_session_t sess; + clitest_buf_t out; + clitest_buf_t err; + int exit_code = -999; + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, resolve_xce_test_path(xce_test_path, sizeof(xce_test_path)), + "must be able to resolve the sibling xce_test binary's path"); + + clitest_buf_init(&out); + clitest_buf_init(&err); + + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, clitest_spawn(&sess, xce_test_path, argv), + "clitest_spawn must launch xce_test behind a real pty"); + + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "xce>", false, 5000), + "expected the initial 'xce>' interactive prompt on stdout"); + + /* `set` only exists in the top-level context's command table -- must + * be issued before entering "client". */ + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "set -y")); + + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "client")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "xce/client>", false, 5000), + "expected to enter the client context"); + + /* With the toggle set, hostctrl -c must proceed WITHOUT prompting even + * though no --yes flag is given and this is a genuinely interactive + * pty session (Requirement 9.6/9.7). */ + TEST_ASSERT_EQUAL_INT(0, + clitest_send_line(&sess, "hostctrl -h node01 -c")); + TEST_ASSERT_EQUAL_INT( + 0, clitest_send_line(&sess, "echo marker_toggle_on_done")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "marker_toggle_on_done", + false, 5000), + "hostctrl -c must run to completion without blocking on a prompt " + "while the session-scoped skip toggle is set (Requirement 9.6)"); + TEST_ASSERT_NULL_MESSAGE( + strstr(err.data, "About to"), + "the session-scoped skip toggle must suppress the confirmation " + "prompt regardless of TTY state (Requirement 9.6, 9.7)"); + + /* `set +y` must restore the default prompting behavior. `set` only + * exists in the top-level context's command table (see the "set -y" + * comment above) -- leave the client context first, exactly like any + * other command sequence that needs a top-level-only command + * mid-session. */ + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "..")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "xce>", false, 5000), + "expected to return to the top-level context after '..'"); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "set +y")); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "client")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "xce/client>", false, 5000), + "expected to re-enter the client context"); + + clitest_buf_free(&err); + clitest_buf_init(&err); + TEST_ASSERT_EQUAL_INT(0, + clitest_send_line(&sess, "hostctrl -h node01 -c")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "About to close host node01. Continue?", true, + 5000), + "'set +y' must restore confirmation prompting (Requirement 9.7)"); + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "n")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "not run", true, 5000), + "declining after the toggle is restored must report not run"); + + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "quit")); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 0, clitest_wait_for_exit(&sess, &out, &err, 5000, &exit_code), + "xce_test did not exit within the harness's safety timeout after " + "'quit'"); + + clitest_kill_and_reap(&sess); + clitest_close(&sess); + clitest_buf_free(&out); + clitest_buf_free(&err); +} + void test_test_cli(void) { UnitySetTestFile(__FILE__); @@ -1912,4 +2322,7 @@ void test_test_cli(void) RUN_TEST(test_interrupt_abandons_blocked_rpc_well_before_timeout); RUN_TEST(test_interrupt_at_empty_prompt_discards_line_without_exiting); RUN_TEST(test_interrupt_then_immediate_teardown_no_uaf); + RUN_TEST(test_qctrl_hostctrl_confirm_gate_destructiveness_determination); + RUN_TEST(test_destructive_commands_prompt_by_default_and_yes_bypasses); + RUN_TEST(test_confirm_gate_session_toggle_skips_and_restores); }