diff --git a/.kiro/specs/xce-test-cli-ux/tasks.md b/.kiro/specs/xce-test-cli-ux/tasks.md index 820ae79b..73e14ebd 100644 --- a/.kiro/specs/xce-test-cli-ux/tasks.md +++ b/.kiro/specs/xce-test-cli-ux/tasks.md @@ -98,7 +98,7 @@ - _Boundary: Bug-1557 Fix_ - [ ] 3. Integration: migrate existing commands onto the new foundations -- [ ] 3.1 Migrate every flag-accepting command's parsing to the shared validation library +- [x] 3.1 Migrate every flag-accepting command's parsing to the shared validation library - Convert every command currently parsing flags via the older mechanisms (including the one command using a fully hand-rolled long-flag parser) to the shared descriptor-table-driven parsing built in 1.1/1.2, so every one uniformly rejects an unrecognized or malformed flag and gains a long-form equivalent for each concept - While migrating the queue-control command, initialize its action selection to an explicit unset sentinel and reject the invocation if none of its mutually-exclusive action flags was given, matching its sibling host-control command's existing pattern - Observable completion: every migrated command rejects a deliberately invalid flag with a usage summary and, where applicable, a suggestion, and the queue-control command rejects an invocation given none of its action flags instead of transmitting an indeterminate value @@ -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) +- 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) - When a fix touches a pure function stuck inside `test.c` (no networking/REPL/global-state dependency), extract it into its own module with a standalone meson `test()` target rather than settling for a throwaway/manual verification — this codebase already has two precedents for exactly this (`resval_typeresolve.c`, `misc.c`'s runusage helpers; now also `joblist_display.c` for `worker_status_str`/`job_state_str`/`format_job_time`). `test.c` itself is never linked into `controller_unit_test`, so this extract-to-standalone-module pattern is the only way to get permanent automated coverage for such functions — a scratch/deleted verification harness is not sufficient and will be sent back on review. `cmd_reslist`/`joblist_query`/`cmd_getconf`'s table-printing bodies remain genuinely untestable without a live cluster (fetch+print are inlined together, confirmed on review) and stay uncovered by permanent tests for now. (task 2.5) - `cmd_source` translates `rcfile_load`'s raw positive "aborted" return to `-1` rather than forwarding it verbatim, because `preprocess_and_run` only treats a *negative* return as failure (`if (rc < 0)`) — forwarding the raw positive value would let an enclosing `source` line silently read a transitively-aborted nested source as success. No test yet exercises this specific nested-abort-propagation case (all 3 required scenarios pass either way) — worth a dedicated test in Validation phase (4.3 or nearby). (task 2.4) diff --git a/.wolf/buglog.json b/.wolf/buglog.json index cc759169..779fe569 100644 --- a/.wolf/buglog.json +++ b/.wolf/buglog.json @@ -19290,7 +19290,7 @@ "error_message": "cmd_qctrl declares `int action;` with no initializer and only assigns it inside the -a/-i/-o/-c getopt cases; there is no post-loop check that one of them was given before `queuectrl.action = action` is sent over the wire. Running `qctrl -q normal` with no action flag (syntactically valid per USAGE_QCTRL, which lists all four as optional) transmits indeterminate stack memory as the queue action.", "file": "src/controller/testsuite/test.c:2663 (declaration), :2668-2688 (getopt loop), :2695 (queuectrl.action = action)", "root_cause": "Missing initializer + missing post-loop validation, unlike the very next sibling command cmd_hostctrl which initializes action=HOST_DO_NONE and explicitly rejects that sentinel.", - "fix": "NOT YET FIXED. Recommended: initialize action to a sentinel (e.g. -1) and reject with a clear error if unset after the getopt loop, mirroring cmd_hostctrl's existing pattern.", + "fix": "FIXED (2026-09-09, xce-test-cli-ux task 3.1, flagspec migration). cmd_qctrl now initializes action = QUEUE_DO_NONE (xce/types.h pre-existing sentinel, same enum family as HOST_DO_NONE) before the getopt_long() loop, and rejects the invocation post-loop if action is still QUEUE_DO_NONE after parsing -- mirroring cmd_hostctrl's exact HOST_DO_NONE pattern. Landed as part of migrating cmd_qctrl (and 16 other flag-accepting commands) from getopt() to the shared flagspec_t + getopt_long() mechanism (flagspec.c/.h, tasks 1.1/1.2). Regression-locked by test_qctrl_missing_action_flag_rejected and test_qctrl_flag_forms_short_and_long_both_accepted in src/controller/testsuite/test_test_cli.c.", "tags": [ "controller", "testsuite", @@ -19299,11 +19299,11 @@ "uninitialized-variable", "undefined-behavior", "cli-ux-audit", - "unfixed" + "fixed" ], "related_bugs": [], "occurrences": 1, - "last_seen": "2026-08-26T09:32:18Z" + "last_seen": "2026-09-09T00:00:00Z" }, { "id": "bug-1705", diff --git a/src/controller/meson.build b/src/controller/meson.build index b45ed21a..42b52a07 100644 --- a/src/controller/meson.build +++ b/src/controller/meson.build @@ -435,6 +435,8 @@ test('joblist display test', joblist_display_unit_test, join_worker_args_unit_test_src = [ 'testsuite/test_join_worker_args.c', 'testsuite/join_worker_args.c', + 'testsuite/flagspec.c', # xce-test-cli-ux task 3.1: join_worker_parse_attrs + # migrated onto flagspec_build_opts/flagspec_reject ] join_worker_args_unit_test = executable('join_worker_args_unit_test', diff --git a/src/controller/testsuite/join_worker_args.c b/src/controller/testsuite/join_worker_args.c index 3787f746..0ed93ad4 100644 --- a/src/controller/testsuite/join_worker_args.c +++ b/src/controller/testsuite/join_worker_args.c @@ -9,16 +9,47 @@ #include #include #include -#include /* getopt, optarg, optind */ +#include /* getopt_long, optarg, optind */ + +#include "flagspec.h" /* shared flagspec_t descriptor-table flag parsing + + * uniform rejection (xce-test-cli-ux tasks 1.1/1.2), + * migrated onto by task 3.1 */ + +/* Every flag's existing short letter, accepted value, and default are + * unchanged by this migration (Requirement 6.3) -- only a long-form + * equivalent and uniform rejection of an unrecognized/malformed flag are + * added. Long names reuse the same concept spellings used elsewhere in + * xce_test for the identical real-world concepts (e.g. "host"/"jobid" in + * test.c's own migrated commands); the ones here (cpufactor, maxcpus, + * maxmem, maxswap, maxtmp, ndisks) are unique to `join` itself. */ +static const flagspec_t join_flags[] = { + { 'f', "cpufactor", "cpufactor", required_argument, 0, "1.0" }, + { 'c', "maxcpus", "maxcpus", required_argument, 0, "4" }, + { 'm', "maxmem", "maxmem", required_argument, 0, + "64 * 1024 * 1024 (64 MB in bytes)" }, + { 's', "maxswap", "maxswap", required_argument, 0, "1024" }, + { 't', "maxtmp", "maxtmp", required_argument, 0, "2048" }, + { 'd', "ndisks", "ndisks", required_argument, 0, "1" }, +}; +#define JOIN_NFLAGS (sizeof(join_flags) / sizeof(join_flags[0])) int join_worker_parse_attrs(int argc, char* argv[], hostattrs_t* attrs) { - int opt; + int opt; + struct option long_opts[JOIN_NFLAGS + 1]; + char optstring[16]; if (attrs == NULL) { return -EINVAL; } + if (flagspec_build_opts(join_flags, JOIN_NFLAGS, long_opts, optstring, + sizeof(optstring)) + < 0) { + fprintf(stderr, "xce_test: join: internal error building flag table\n"); + return -EINVAL; + } + /* bug-1557: maxmem is bytes-canonical (see resources.yaml, hardware_ * monitor.c's total_memory) -- this default used to read plain "64" * back when maxmem was MB-scale (64 MB); an earlier session converted @@ -38,8 +69,10 @@ int join_worker_parse_attrs(int argc, char* argv[], hostattrs_t* attrs) attrs->hosttype = "LINUX64"; optind = 1; + opterr = 0; - while ((opt = getopt(argc, argv, "f:c:m:s:t:d:")) != -1) { + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'f': attrs->cpufactor = atof(optarg); @@ -59,10 +92,11 @@ int join_worker_parse_attrs(int argc, char* argv[], hostattrs_t* attrs) case 'd': attrs->ndisks = atoi(optarg); break; - default: - printf(" join [-f cpufactor] [-c maxcpus] [-m maxmem] [-s maxswap] " - "[-t maxtmp] [-d ndisks]\n"); - return -EINVAL; + case '?': + return flagspec_reject( + "join", join_flags, JOIN_NFLAGS, argv[optind - 1], + "join [-f ] [-c ] [-m ] " + "[-s ] [-t ] [-d ]"); } } diff --git a/src/controller/testsuite/test.c b/src/controller/testsuite/test.c index bb810cf9..67d1a369 100644 --- a/src/controller/testsuite/test.c +++ b/src/controller/testsuite/test.c @@ -25,6 +25,11 @@ #include "controller.capnp.h" #include "ctrlstatus_fmt.h" /* ctrlstatus_format_time_ms (task 6.3) */ #include "delay.h" +#include "flagspec.h" /* shared flagspec_t descriptor-table flag parsing, + * rejection (with typo suggestion), and help + * rendering (xce-test-cli-ux tasks 1.1/1.2); every + * flag-accepting command below migrates onto this + * (task 3.1) */ #include "idgen.h" #include "preason.h" #include "walcompare.h" /* cross-member committed-prefix divergence (cmd_walcompare) */ @@ -1311,39 +1316,77 @@ int cmd_waldump(int argc, char* argv[]) * committed-vs-uncommitted classification for each divergence (task 1.1). Exit * 0 = identical, nonzero = divergence or load error — so it doubles as a * scripted assertion. */ +/* Both flags are long-only (no short letter ever existed for either -- + * cmd_walcompare was the one command using a fully hand-rolled long-flag + * parser, per xce-test-cli-ux task 3.1's explicit scope). Their synthetic + * getopt_long() `val`s are FLAGSPEC_LONG_ONLY_BASE + + * (flagspec.h) -- the enum below names those indices so the switch below + * doesn't rely on bare magic numbers. */ +enum { WALCOMPARE_OPT_REPORT = 0, WALCOMPARE_OPT_COMMITTED = 1 }; + +static const flagspec_t walcompare_flags[] = { + { 0, "report", "report", no_argument, 0, NULL }, + { 0, "committed", "committed", required_argument, 0, + "derive the shared boundary from members' CRC-intact tips" }, +}; +#define WALCOMPARE_NFLAGS \ + (sizeof(walcompare_flags) / sizeof(walcompare_flags[0])) + int cmd_walcompare(int argc, char* argv[]) { - uint64_t committed = 0; /* 0 = derive the shared boundary */ - int report = 0; /* --report => full per-member machine table */ - int ai = 1; + uint64_t committed = 0; /* 0 = derive the shared boundary */ + int report = 0; /* --report => full per-member machine table */ + int opt; + struct option long_opts[WALCOMPARE_NFLAGS + 1]; + char optstring[8]; + char posix_optstring[sizeof(optstring) + 1]; - /* Optional flags (any order): --report, --committed N. */ - for (;;) { - if (argc > ai && strcmp(argv[ai], "--report") == 0) { + optind = 1; + opterr = 0; + + if (flagspec_build_opts(walcompare_flags, WALCOMPARE_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: walcompare: internal error building flag table\n"); + return -EINVAL; + } + + /* Leading '+' (GNU getopt extension): stop at the first non-option + * argument instead of permuting argv -- matches the pre-migration + * hand-rolled parser's exact behavior (flags recognized only before the + * positional member paths; both flags here are long-only, so the built + * optstring is empty and this is simply "+"). */ + posix_optstring[0] = '+'; + strcpy(posix_optstring + 1, optstring); + + while ((opt = getopt_long(argc, argv, posix_optstring, long_opts, NULL)) + != -1) { + switch (opt) { + case FLAGSPEC_LONG_ONLY_BASE + WALCOMPARE_OPT_REPORT: report = 1; - ai += 1; - continue; - } - if (argc > ai && strcmp(argv[ai], "--committed") == 0) { - if (argc <= ai + 1) { - fprintf(stderr, "walcompare: --committed needs an LSN\n"); - return -1; - } + break; + case FLAGSPEC_LONG_ONLY_BASE + WALCOMPARE_OPT_COMMITTED: { char* end = NULL; - unsigned long v = strtoul(argv[ai + 1], &end, 10); - if (end == argv[ai + 1] || *end != '\0') { + unsigned long v = strtoul(optarg, &end, 10); + if (end == optarg || *end != '\0') { fprintf(stderr, "walcompare: bad --committed LSN '%s'\n", - argv[ai + 1]); + optarg); return -1; } committed = (uint64_t)v; - ai += 2; - continue; + break; + } + case '?': + return flagspec_reject( + "walcompare", walcompare_flags, WALCOMPARE_NFLAGS, + argv[optind - 1], + "walcompare [--report] [--committed ] " + " [more...]"); } - break; } - int n = argc - ai; + int n = argc - optind; if (n < 2) { fprintf(stderr, "usage: walcompare [--report] [--committed N] " @@ -1357,7 +1400,7 @@ int cmd_walcompare(int argc, char* argv[]) walcompare_member_t members[16]; for (int i = 0; i < n; i++) { - members[i].path = argv[ai + i]; + members[i].path = argv[optind + i]; members[i].committed_lsn = committed; /* 0 => derive shared boundary */ } @@ -1661,16 +1704,33 @@ int cmd_quit(int argc, char* argv[]) return 0; } +static const flagspec_t hello_flags[] = { + { 'c', "count", "count", required_argument, 0, "1" }, + { 'm', "message", "message", required_argument, 0, + "\"Greeting from xce client\"" }, +}; +#define HELLO_NFLAGS (sizeof(hello_flags) / sizeof(hello_flags[0])) + int cmd_hello(int argc, char* argv[]) { - int rc = 0; - int opt; - char* msg = "Greeting from xce client"; - int count = 1; + int rc = 0; + int opt; + char* msg = "Greeting from xce client"; + int count = 1; + struct option long_opts[HELLO_NFLAGS + 1]; + char optstring[32]; optind = 1; + opterr = 0; - while ((opt = getopt(argc, argv, "c:m:")) != -1) { + if (flagspec_build_opts(hello_flags, HELLO_NFLAGS, long_opts, optstring, + sizeof(optstring)) + < 0) { + fprintf(stderr, "xce_test: hello: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) != -1) { switch (opt) { case 'c': count = atoi(optarg); @@ -1678,13 +1738,10 @@ int cmd_hello(int argc, char* argv[]) case 'm': msg = optarg; break; - default: - /* -w was never implemented (getopt string above is "c:m:" only) - * -- this hint previously advertised a nonexistent -w wait_usec - * flag; corrected to match the actual accepted options. */ - fprintf(stderr, " hello [-c count] [-m message]\n"); - return -1; - break; + case '?': + return flagspec_reject("hello", hello_flags, HELLO_NFLAGS, + argv[optind - 1], + "hello [-c ] [-m ]"); } } @@ -2335,13 +2392,30 @@ static void print_jobinfo(xce_job_t* job) } } +static const flagspec_t jobinfo_flags[] = { + { 'j', "jobid", "jobid", required_argument, 1, NULL }, +}; +#define JOBINFO_NFLAGS (sizeof(jobinfo_flags) / sizeof(jobinfo_flags[0])) + int cmd_jobinfo(int argc, char* argv[]) { - int opt; + int opt; + struct option long_opts[JOBINFO_NFLAGS + 1]; + char optstring[16]; optind = 1; + opterr = 0; - while ((opt = getopt(argc, argv, "j:")) != -1) { + if (flagspec_build_opts(jobinfo_flags, JOBINFO_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: jobinfo: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { uint64_t jobid; xce_job_t* job; int rc = 0; @@ -2362,6 +2436,10 @@ int cmd_jobinfo(int argc, char* argv[]) } break; + case '?': + return flagspec_reject("jobinfo", jobinfo_flags, JOBINFO_NFLAGS, + argv[optind - 1], + "jobinfo -j [-j ...]"); } } return 0; @@ -2409,6 +2487,12 @@ static int fill_job_alloc(xce_job_alloc_t* jalloc, int nhosts, return 0; } +static const flagspec_t jobrun_flags[] = { + { 'm', "host", "host", required_argument, 0, "1 task; repeatable up to 5" }, + { 'j', "jobid", "jobid", required_argument, 1, NULL }, +}; +#define JOBRUN_NFLAGS (sizeof(jobrun_flags) / sizeof(jobrun_flags[0])) + int cmd_jobrun(int argc, char* argv[]) { int rc; @@ -2418,13 +2502,25 @@ int cmd_jobrun(int argc, char* argv[]) xce_job_alloc_t jalloc; xce_job_dispatch_t jdispatch = { .njobs = 1, .allocs = (xce_job_alloc_t*[]) { &jalloc } }; - int nhosts = 0; - char* host; - int count; + int nhosts = 0; + char* host; + int count; + struct option long_opts[JOBRUN_NFLAGS + 1]; + char optstring[16]; optind = 1; + opterr = 0; - while ((opt = getopt(argc, argv, "m:j:")) != -1) { + if (flagspec_build_opts(jobrun_flags, JOBRUN_NFLAGS, long_opts, optstring, + sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: jobrun: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'm': if (nhosts >= MAX_TASK_HOSTS) { @@ -2460,6 +2556,10 @@ int cmd_jobrun(int argc, char* argv[]) return -1; } break; + case '?': + return flagspec_reject( + "jobrun", jobrun_flags, JOBRUN_NFLAGS, argv[optind - 1], + "jobrun -j [-m [:]] [-m ...]"); } } @@ -2510,6 +2610,18 @@ static int build_job_exdata(xce_job_submit_t* submit) return 0; } +static const flagspec_t jobsub_flags[] = { + { 'u', "user", "user", required_argument, 1, NULL }, + { 'q', "queue", "queue", required_argument, 1, NULL }, + { 'R', "resreq", "resreq", required_argument, 0, + "\"type == any\"" }, + { 'c', "command-line", "command-line", required_argument, 1, NULL }, + { 'n', "ntasks", "ntasks", required_argument, 0, "1" }, + { 'x', "exdata", "exdata", required_argument, 0, NULL }, + { 'a', "array-size", "arraysize", required_argument, 0, "1" }, +}; +#define JOBSUB_NFLAGS (sizeof(jobsub_flags) / sizeof(jobsub_flags[0])) + int cmd_jobsub(int argc, char* argv[]) { int rc = 0; @@ -2524,8 +2636,11 @@ int cmd_jobsub(int argc, char* argv[]) uint32_t min = 1, max = 1; uint32_t arsize = 1; char jobidStr[128]; + struct option long_opts[JOBSUB_NFLAGS + 1]; + char optstring[32]; optind = 1; + opterr = 0; memset(&submit, 0x0, sizeof(submit)); submit.start_info @@ -2535,7 +2650,16 @@ int cmd_jobsub(int argc, char* argv[]) return -ENOMEM; } - while ((opt = getopt(argc, argv, "u:q:R:c:x:n:a:")) != -1) { + if (flagspec_build_opts(jobsub_flags, JOBSUB_NFLAGS, long_opts, optstring, + sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: jobsub: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'u': submit.user = strdup(optarg); @@ -2578,10 +2702,11 @@ int cmd_jobsub(int argc, char* argv[]) arsize = 1; } break; - default: - printf( - "jobsub -u -q -R -c \n"); - rc = -EINVAL; + case '?': + rc = flagspec_reject( + "jobsub", jobsub_flags, JOBSUB_NFLAGS, argv[optind - 1], + "jobsub -u -q -c [-R ] " + "[-n [,]] [-x ] [-a ]"); goto cleanup; } } @@ -2790,6 +2915,13 @@ static void format_bytes_human(double bytes, char* buf, size_t bufsize) snprintf(buf, bufsize, "%.2f %s", val, UNITS[i]); } +static const flagspec_t resval_flags[] = { + { 'n', "resname", "resname", required_argument, 1, NULL }, + { 'm', "host", "host", required_argument, 0, + "required unless the resource is cluster-wide" }, +}; +#define RESVAL_NFLAGS (sizeof(resval_flags) / sizeof(resval_flags[0])) + int cmd_resval(int argc, char* argv[]) { char* host = NULL; @@ -2797,10 +2929,22 @@ int cmd_resval(int argc, char* argv[]) xce_resval_t** vals = NULL; int opt; int rc; + struct option long_opts[RESVAL_NFLAGS + 1]; + char optstring[16]; optind = 1; + opterr = 0; - while ((opt = getopt(argc, argv, "n:m:")) != -1) { + if (flagspec_build_opts(resval_flags, RESVAL_NFLAGS, long_opts, optstring, + sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: resval: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'n': res = strdup(optarg); @@ -2808,11 +2952,13 @@ int cmd_resval(int argc, char* argv[]) case 'm': host = strdup(optarg); break; - default: - fprintf(stderr, "resval -n resname [-m host]\n"); + case '?': + rc = flagspec_reject("resval", resval_flags, RESVAL_NFLAGS, + argv[optind - 1], + "resval -n [-m ]"); FREEUP(host); FREEUP(res); - return -1; + return rc; } } @@ -2863,6 +3009,12 @@ int cmd_resval(int argc, char* argv[]) return 0; } +static const flagspec_t jobkill_flags[] = { + { 's', "signal", "signal", required_argument, 0, "SIGTERM" }, + { 'j', "jobid", "jobid", required_argument, 1, NULL }, +}; +#define JOBKILL_NFLAGS (sizeof(jobkill_flags) / sizeof(jobkill_flags[0])) + int cmd_jobkill(int argc, char* argv[]) { int signo; @@ -2872,11 +3024,23 @@ int cmd_jobkill(int argc, char* argv[]) xce_jobctrl_t jobctrl; xce_signal_t siginfo; char jobidStr[64]; + struct option long_opts[JOBKILL_NFLAGS + 1]; + char optstring[16]; optind = 1; + opterr = 0; signo = SIGTERM; - while ((opt = getopt(argc, argv, "s:j:")) != -1) { + if (flagspec_build_opts(jobkill_flags, JOBKILL_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: jobkill: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 's': signo = atoi(optarg); @@ -2884,8 +3048,10 @@ int cmd_jobkill(int argc, char* argv[]) case 'j': parse_jobid(&jobid, optarg); break; - default: - break; + case '?': + return flagspec_reject("jobkill", jobkill_flags, JOBKILL_NFLAGS, + argv[optind - 1], + "jobkill -j [-s ]"); } } @@ -2912,6 +3078,16 @@ int cmd_jobkill(int argc, char* argv[]) return 0; } +static const flagspec_t jobckpt_flags[] = { + { 'k', "kill", "kill", no_argument, 0, + NULL }, + { 'j', "jobid", "jobid", required_argument, 1, + NULL }, + { 'm', "checkpoint-method", "checkpoint-method", required_argument, 0, + "\"default\"" }, +}; +#define JOBCKPT_NFLAGS (sizeof(jobckpt_flags) / sizeof(jobckpt_flags[0])) + int cmd_jobckpt(int argc, char* argv[]) { uint64_t jobid = 0; @@ -2920,11 +3096,23 @@ int cmd_jobckpt(int argc, char* argv[]) xce_jobctrl_t jobctrl; xce_ckpt_options_t ckptoption; char jobidStr[64]; + struct option long_opts[JOBCKPT_NFLAGS + 1]; + char optstring[16]; optind = 1; + opterr = 0; ckptoption.kill = 0; - while ((opt = getopt(argc, argv, "kj:m:")) != -1) { + if (flagspec_build_opts(jobckpt_flags, JOBCKPT_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: jobckpt: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'k': ckptoption.kill = 1; @@ -2935,8 +3123,10 @@ int cmd_jobckpt(int argc, char* argv[]) case 'm': ckptoption.method = optarg; break; - default: - break; + case '?': + return flagspec_reject("jobckpt", jobckpt_flags, JOBCKPT_NFLAGS, + argv[optind - 1], + "jobckpt -j [-k] [-m ]"); } } @@ -2964,17 +3154,44 @@ int cmd_jobckpt(int argc, char* argv[]) return 0; } +static const flagspec_t qctrl_flags[] = { + { 'a', "activate", "activate", no_argument, 0, NULL }, + { 'i', "deactivate", "deactivate", no_argument, 0, NULL }, + { 'o', "open", "open", no_argument, 0, NULL }, + { 'c', "close", "close", no_argument, 0, NULL }, + { 'q', "queue", "queue", required_argument, 1, NULL }, +}; +#define QCTRL_NFLAGS (sizeof(qctrl_flags) / sizeof(qctrl_flags[0])) + int cmd_qctrl(int argc, char* argv[]) { int opt; int rc; xce_queuectrl_t queuectrl; - int action; + /* bug-1704 fix: initialize to the QUEUE_DO_NONE sentinel (xce/types.h) + * and reject post-loop if it is still unset -- mirrors cmd_hostctrl's + * existing HOST_DO_NONE sentinel-and-reject pattern immediately below. + * Before this fix, `action` had no initializer and no post-loop check, + * so an invocation giving -q but none of -a/-i/-o/-c transmitted + * indeterminate stack memory as the queue action. */ + int action = QUEUE_DO_NONE; char* queue = NULL; + struct option long_opts[QCTRL_NFLAGS + 1]; + char optstring[16]; optind = 1; + opterr = 0; - while ((opt = getopt(argc, argv, "aiociq:")) != -1) { + if (flagspec_build_opts(qctrl_flags, QCTRL_NFLAGS, long_opts, optstring, + sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: qctrl: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'a': action = QUEUE_ACTIVE; @@ -2991,8 +3208,11 @@ int cmd_qctrl(int argc, char* argv[]) case 'q': queue = strdup(optarg); break; - default: - break; + case '?': + FREEUP(queue); + return flagspec_reject("qctrl", qctrl_flags, QCTRL_NFLAGS, + argv[optind - 1], + "qctrl -q [-a] [-i] [-o] [-c]"); } } @@ -3001,6 +3221,16 @@ int cmd_qctrl(int argc, char* argv[]) return -1; } + /* bug-1704 fix (continued): reject rather than send an indeterminate + * action when none of -a/-i/-o/-c (nor their long forms) was given. */ + if (action == QUEUE_DO_NONE) { + fprintf(stderr, + "you need to specify one of -a/--activate, -i/--deactivate, " + "-o/--open, or -c/--close\n"); + free(queue); + return -1; + } + queuectrl.action = action; queuectrl.queue = queue; @@ -3024,17 +3254,36 @@ int cmd_qctrl(int argc, char* argv[]) * but are rejected -EINVAL server-side today, so no flag is offered for them * here). Used by the config-upgrade chaotic soak (task 8.6) to set a host's * admin status before/around a config upgrade and assert it survives. */ +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 }, +}; +#define HOSTCTRL_NFLAGS (sizeof(hostctrl_flags) / sizeof(hostctrl_flags[0])) + int cmd_hostctrl(int argc, char* argv[]) { int opt; int rc; xce_hostctrl_t hostctrl; - int action = HOST_DO_NONE; + int action = HOST_DO_NONE; char* hostname = NULL; + struct option long_opts[HOSTCTRL_NFLAGS + 1]; + char optstring[16]; optind = 1; + opterr = 0; - while ((opt = getopt(argc, argv, "och:")) != -1) { + if (flagspec_build_opts(hostctrl_flags, HOSTCTRL_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: hostctrl: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'o': action = HOST_OPEN; @@ -3045,8 +3294,11 @@ int cmd_hostctrl(int argc, char* argv[]) case 'h': hostname = strdup(optarg); break; - default: - break; + case '?': + FREEUP(hostname); + return flagspec_reject("hostctrl", hostctrl_flags, + HOSTCTRL_NFLAGS, argv[optind - 1], + "hostctrl -h (-o|-c)"); } } @@ -3150,6 +3402,19 @@ int cmd_upgradeconfig(int argc, char* argv[]) return 0; } +static const flagspec_t runtasks_flags[] = { + { 'm', "host", "host", required_argument, 0, + "1 task; repeatable up to 5" }, + { 'u', "user", "user", required_argument, 0, NULL }, + { 'c', "command-line", "command-line", required_argument, 0, NULL }, + { 'x', "exdata", "exdata", required_argument, 0, NULL }, + { 'd', "dir", "dir", required_argument, 0, NULL }, + { 'j', "jobid", "jobid", required_argument, 0, NULL }, + { 't', "launch-type", "launch-type", required_argument, 0, + "\"mpi\" if >1 total tasks, else \"sequential\"" }, +}; +#define RUNTASKS_NFLAGS (sizeof(runtasks_flags) / sizeof(runtasks_flags[0])) + int cmd_runtasks(int argc, char* argv[]) { xce_taskhost_t hosts[MAX_TASK_HOSTS]; @@ -3167,10 +3432,22 @@ int cmd_runtasks(int argc, char* argv[]) char* launch_type = NULL; int i; uint64_t jobid = 0; + struct option long_opts[RUNTASKS_NFLAGS + 1]; + char optstring[32]; optind = 1; + opterr = 0; - while ((opt = getopt(argc, argv, "m:u:c:x:d:j:t:")) != -1) { + if (flagspec_build_opts(runtasks_flags, RUNTASKS_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: taskrun: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'm': if (nhosts >= MAX_TASK_HOSTS) { @@ -3229,8 +3506,12 @@ int cmd_runtasks(int argc, char* argv[]) case 't': launch_type = strdup(optarg); break; - default: - break; + case '?': + return flagspec_reject( + "taskrun", runtasks_flags, RUNTASKS_NFLAGS, + argv[optind - 1], + "taskrun [-j ] [-m [:]]... [-u ] " + "[-c ] [-x ] [-d ] [-t ]"); } } @@ -3311,6 +3592,12 @@ static int add_pendreason(xce_job_preason_t* preason, char* s) return add_job_preason(preason, code, phost, pres); } +static const flagspec_t setpend_flags[] = { + { 'j', "jobid", "jobid", required_argument, 1, NULL }, + { 'p', "reason", "reason", required_argument, 1, NULL }, +}; +#define SETPEND_NFLAGS (sizeof(setpend_flags) / sizeof(setpend_flags[0])) + int cmd_setpend(int argc, char* argv[]) { int opt; @@ -3318,11 +3605,23 @@ int cmd_setpend(int argc, char* argv[]) xce_pendreason_t preason = { .reasons = &jreasons }; xce_preason_set_t reasons = { .nreasons = 1, .reasons = (xce_pendreason_t*[]) { &preason } }; - int rc = 0; + int rc = 0; + struct option long_opts[SETPEND_NFLAGS + 1]; + char optstring[16]; optind = 1; + opterr = 0; - while ((opt = getopt(argc, argv, "j:p:")) != -1) { + if (flagspec_build_opts(setpend_flags, SETPEND_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: setpend: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { int rc = 0; switch (opt) { case 'j': @@ -3335,6 +3634,10 @@ int cmd_setpend(int argc, char* argv[]) return rc; } break; + case '?': + return flagspec_reject( + "setpend", setpend_flags, SETPEND_NFLAGS, argv[optind - 1], + "setpend -j -p [-p ...]"); } } @@ -3405,6 +3708,13 @@ int cmd_join_worker(int argc, char* argv[]) * apply_committed_entry()) that a real worker's report would, without * requiring a real scheduler dispatch (on_job_start only requires the job to * already exist in jobmgr, i.e. have been submitted). */ +static const flagspec_t jobstart_flags[] = { + { 'j', "jobid", "jobid", required_argument, 1, NULL }, + { 'p', "pid", "pid", required_argument, 0, + "this xce_test process's own pid" }, +}; +#define JOBSTART_NFLAGS (sizeof(jobstart_flags) / sizeof(jobstart_flags[0])) + int cmd_jobstart(int argc, char* argv[]) { int rc; @@ -3413,11 +3723,23 @@ int cmd_jobstart(int argc, char* argv[]) unsigned int pid = 0; unsigned int pids[1]; job_pidinfo_t info; + struct option long_opts[JOBSTART_NFLAGS + 1]; + char optstring[16]; optind = 1; + opterr = 0; memset(&info, 0x0, sizeof(info)); - while ((opt = getopt(argc, argv, "j:p:")) != -1) { + if (flagspec_build_opts(jobstart_flags, JOBSTART_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: jstart: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'j': parse_jobid(&jobid, optarg); @@ -3425,9 +3747,10 @@ int cmd_jobstart(int argc, char* argv[]) case 'p': pid = (unsigned int)atoi(optarg); break; - default: - printf(" jstart -j jobid [-p pid]\n"); - return -EINVAL; + case '?': + return flagspec_reject("jstart", jobstart_flags, JOBSTART_NFLAGS, + argv[optind - 1], + "jstart -j [-p ]"); } } @@ -3456,6 +3779,12 @@ int cmd_jobstart(int argc, char* argv[]) return 0; } +static const flagspec_t jobfinish_flags[] = { + { 'j', "jobid", "jobid", required_argument, 1, NULL }, + { 'c', "exit-code", "exit-code", required_argument, 0, "0" }, +}; +#define JOBFINISH_NFLAGS (sizeof(jobfinish_flags) / sizeof(jobfinish_flags[0])) + int cmd_jobfinish(int argc, char* argv[]) { int rc; @@ -3463,6 +3792,8 @@ int cmd_jobfinish(int argc, char* argv[]) uint64_t jobid = 0; int code = 0; xce_jobfinish_t finish; + struct option long_opts[JOBFINISH_NFLAGS + 1]; + char optstring[16]; /* raft-apply-decoupling task 7.2: reset optind so repeated jfinish * invocations within one xce_test process (the fact-stream soak scenario @@ -3474,8 +3805,18 @@ int cmd_jobfinish(int argc, char* argv[]) * cmd_ handlers in this file already follow (e.g. cmd_join_worker just * above, cmd_jobsub, cmd_resupdate). */ optind = 1; + opterr = 0; - while ((opt = getopt(argc, argv, "j:c:")) != -1) { + if (flagspec_build_opts(jobfinish_flags, JOBFINISH_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: jfinish: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'j': parse_jobid(&jobid, optarg); @@ -3483,9 +3824,10 @@ int cmd_jobfinish(int argc, char* argv[]) case 'c': code = atoi(optarg); break; - default: - printf(" jfinish -j jobid [-c exitcode]\n"); - return -EINVAL; + case '?': + return flagspec_reject("jfinish", jobfinish_flags, + JOBFINISH_NFLAGS, argv[optind - 1], + "jfinish -j [-c ]"); } } @@ -3516,6 +3858,14 @@ int cmd_jobfinish(int argc, char* argv[]) * -> apply_job_fail_transition() at apply time. Reuses the existing * job_failinfo_t shape already exercised internally by this file's own * notify_job_fail() helper (used by run_job() on a local exec failure). */ +static const flagspec_t jobfail_flags[] = { + { 'j', "jobid", "jobid", required_argument, 1, NULL }, + { 'c', "failure-code", "failure-code", required_argument, 0, "0" }, + { 's', "stage", "stage", required_argument, 0, + "JOB_EXEC_STAGE_POST_EXEC" }, +}; +#define JOBFAIL_NFLAGS (sizeof(jobfail_flags) / sizeof(jobfail_flags[0])) + int cmd_jobfail(int argc, char* argv[]) { int rc; @@ -3524,11 +3874,23 @@ int cmd_jobfail(int argc, char* argv[]) int code = 0; int stage = JOB_EXEC_STAGE_POST_EXEC; job_failinfo_t failinfo; + struct option long_opts[JOBFAIL_NFLAGS + 1]; + char optstring[16]; optind = 1; + opterr = 0; memset(&failinfo, 0x0, sizeof(failinfo)); - while ((opt = getopt(argc, argv, "j:c:s:")) != -1) { + if (flagspec_build_opts(jobfail_flags, JOBFAIL_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: jfail: internal error building flag table\n"); + return -EINVAL; + } + + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'j': parse_jobid(&jobid, optarg); @@ -3539,9 +3901,10 @@ int cmd_jobfail(int argc, char* argv[]) case 's': stage = atoi(optarg); break; - default: - printf(" jfail -j jobid [-c code] [-s stage]\n"); - return -EINVAL; + case '?': + return flagspec_reject( + "jfail", jobfail_flags, JOBFAIL_NFLAGS, argv[optind - 1], + "jfail -j [-c ] [-s ]"); } } @@ -3758,6 +4121,14 @@ static void resupdate_report_one(char* name, char* host, xce_val_t* sent) } } +static const flagspec_t resupdate_flags[] = { + { 'n', "resname", "resname", required_argument, 1, NULL }, + { 'v', "value", "value", required_argument, 1, NULL }, + { 't', "value-type", "value-type", required_argument, 0, + "numeric unless overridden" }, +}; +#define RESUPDATE_NFLAGS (sizeof(resupdate_flags) / sizeof(resupdate_flags[0])) + int cmd_resupdate(int argc, char* argv[]) { #define MAX_N_RES (16) @@ -3770,15 +4141,28 @@ int cmd_resupdate(int argc, char* argv[]) int type_override = RESVAL_TYPE_UNKNOWN; int opt; int rc; + struct option long_opts[RESUPDATE_NFLAGS + 1]; + char optstring[16]; optind = 1; + opterr = 0; nres = 0; + + if (flagspec_build_opts(resupdate_flags, RESUPDATE_NFLAGS, long_opts, + optstring, sizeof(optstring)) + < 0) { + fprintf(stderr, + "xce_test: resupdate: internal error building flag table\n"); + return -EINVAL; + } + /* -t is deferred: it applies to the pair most recently opened by -n * (design.md's documented order is -n/-v/[-t]), so a pending pair's * set_resval() call is deferred until the NEXT -n or end-of-args -- * not fired immediately on -v as before -- so a trailing -t still has * a chance to be seen for that same pair. */ - while ((opt = getopt(argc, argv, "n:v:t:")) != -1) { + while ((opt = getopt_long(argc, argv, optstring, long_opts, NULL)) + != -1) { switch (opt) { case 'n': if (resname != NULL) { @@ -3827,6 +4211,12 @@ int cmd_resupdate(int argc, char* argv[]) printf("ignore -t `%s' as no res name found\n", optarg); } break; + case '?': + return flagspec_reject( + "resupdate", resupdate_flags, RESUPDATE_NFLAGS, + argv[optind - 1], + "resupdate -n -v [-t ] " + "[-n -v [-t ] ...]"); } } diff --git a/src/controller/testsuite/test_test_cli.c b/src/controller/testsuite/test_test_cli.c index 73bb5b66..2c8bdec7 100644 --- a/src/controller/testsuite/test_test_cli.c +++ b/src/controller/testsuite/test_test_cli.c @@ -1074,6 +1074,366 @@ static void test_source_nested_noncyclic_executes_every_line(void) clitest_fixture_cleanup_dir(); } +/* ------------------------------------------------------------------ */ +/* Flag Parsing Layer migration (task 3.1, Requirements 2.1-2.4, 6.1-6.4, */ +/* 10.1-10.3) */ +/* ------------------------------------------------------------------ */ + +/* + * test_qctrl_missing_action_flag_rejected -- bug-1704's mandatory + * regression test, named explicitly by task 3.1's Observable Completion + * line: `qctrl` given a queue name but NONE of its mutually-exclusive + * action flags (-a/-i/-o/-c, nor their new long forms --activate/ + * --deactivate/--open/--close) must be REJECTED, not silently transmit an + * indeterminate (pre-fix: uninitialized) `action` value over the wire. + * The sentinel-and-reject check (mirroring cmd_hostctrl's pre-existing + * HOST_DO_NONE pattern) runs entirely inside cmd_qctrl before any + * xcecli_queuectrl() network call, so no live controller is needed for + * this assertion to be meaningful. */ +static void test_qctrl_missing_action_flag_rejected(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"); + + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "qctrl -q normal")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "you need to specify one of", true, 5000), + "bug-1704: qctrl given -q but none of -a/-i/-o/-c (nor their long " + "forms) must be rejected with an explicit stderr error, never send " + "an indeterminate action"); + + 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'"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, exit_code, + "the rejected qctrl invocation must mark the session failed"); + + clitest_kill_and_reap(&sess); + clitest_close(&sess); + clitest_buf_free(&out); + clitest_buf_free(&err); +} + +/* + * test_qctrl_flag_forms_short_and_long_both_accepted -- Requirement 6.3 + * companion to the bug-1704 test above: proves the migration did not + * regress a VALID invocation for either qctrl's pre-existing short action + * flag (-a) or its newly added long form (--activate). Both must parse + * past the sentinel-and-reject check and reach the actual + * xcecli_queuectrl() network call (which then fails fast, since no live + * controller is running in this unit-test environment -- that failure + * itself is not under test here, only that parsing succeeded and the + * command was NOT rejected as an unrecognized flag or a missing action). + * A distinct `echo` marker after each qctrl invocation gives a reliable + * command-boundary signal on stdout (REPL commands execute strictly in + * order), so the stderr accumulated by the time each marker appears can be + * inspected without racing the (synchronous, blocking) qctrl call itself. + */ +static void test_qctrl_flag_forms_short_and_long_both_accepted(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"); + + 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_short_form_done")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "marker_qctrl_short_form_done", false, 5000), + "the short-form '-a' invocation must run to completion"); + + TEST_ASSERT_EQUAL_INT( + 0, clitest_send_line(&sess, "qctrl -q normal --activate")); + TEST_ASSERT_EQUAL_INT( + 0, clitest_send_line(&sess, "echo marker_qctrl_long_form_done")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "marker_qctrl_long_form_done", false, 5000), + "the long-form '--activate' invocation must run to completion"); + + TEST_ASSERT_NULL_MESSAGE( + strstr(err.data, "unrecognized flag"), + "neither the short form '-a' nor its long form '--activate' may be " + "rejected as an unrecognized flag (Requirement 6.3: adding a long " + "form must not change the short flag's existing behavior)"); + TEST_ASSERT_NULL_MESSAGE( + strstr(err.data, "you need to specify one of"), + "both '-a' and '--activate' must be recognized as setting the " + "action -- bug-1704's sentinel check must not fire for either"); + + 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_migrated_commands_sample_reject_unrecognized_flag -- representative + * sample (task 3.1's flagspec migration touched ~17 commands; the shared + * flagspec_reject()/flagspec_build_opts() mechanism itself is already + * unit-tested in isolation by tasks 1.1/1.2's test_flagspec.c, so + * exhaustively black-box-testing every migrated command here would be + * disproportionate). This sample specifically covers commands whose + * PRE-MIGRATION behavior on an unrecognized flag was to silently ignore it + * and keep running (jobinfo and jobkill: no default:/case '?' arm at all, + * or a `default: break;` -- research.md's documented finding) -- exactly + * the defect class Requirement 2.1/2.3 exists to close -- plus hostctrl + * (cmd_qctrl's sibling, named explicitly in this task's brief) and + * setpend/resupdate (which had NO default arm whatsoever). Every assertion + * fires before any network call inside each cmd_* handler, so no live + * controller is needed. jobinfo's case additionally proves Requirement + * 10.1's typo suggestion end-to-end ('-z' closely matches '-j'/'--jobid'). + */ +static void test_migrated_commands_sample_reject_unrecognized_flag(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"); + + /* jobinfo: previously had no default:/case '?' arm at all -- an + * unrecognized flag silently fell through and the loop simply moved + * on. Also proves Requirement 10.1's typo suggestion: '-z' is within + * this table's edit-distance threshold of '-j'. */ + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "jobinfo -z")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text( + &sess, &out, &err, + "jobinfo: unrecognized flag '-z' (did you mean '-j/--jobid'?)", + true, 5000), + "jobinfo must reject an unrecognized flag with a usage summary and " + "a typo suggestion, not silently ignore it (Requirements 2.1, " + "10.1)"); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "usage: jobinfo", true, + 5000), + "the rejection must include jobinfo's usage summary (Requirement " + "2.4)"); + + /* jobkill: previously `default: break;` -- an unrecognized flag given + * ALONGSIDE a valid -j was silently ignored and the command still + * executed. Sending it after a valid -j proves the loop now stops and + * rejects at the first bad flag, before ever reaching the network + * call or jobid validation. */ + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "jobkill -j 1 -z")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "jobkill: unrecognized flag '-z'", true, 5000), + "jobkill must reject an unrecognized flag instead of silently " + "ignoring it and proceeding (Requirement 2.1)"); + + /* hostctrl: cmd_qctrl's sibling, named explicitly in this task's own + * brief as already having the correct sentinel pattern this migration + * must preserve; also previously `default: break;`. */ + TEST_ASSERT_EQUAL_INT(0, clitest_send_line(&sess, "hostctrl --bogus")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "hostctrl: unrecognized flag '--bogus'", true, + 5000), + "hostctrl must reject an unrecognized flag (Requirement 2.1)"); + + /* setpend lives in the SCHEDULER context (scheduler_commands[]), not + * client -- leave client and switch contexts before exercising it. + * Previously had NO default:/case '?' arm whatsoever. */ + 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, "scheduler")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "xce/scheduler>", false, + 5000), + "expected to enter the scheduler context"); + + TEST_ASSERT_EQUAL_INT(0, + clitest_send_line(&sess, "setpend -j 1 --bogus")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "setpend: unrecognized flag '--bogus'", true, + 5000), + "setpend must reject an unrecognized flag (Requirement 2.1)"); + + /* resupdate lives in the WORKER context (worker_commands[]), not + * client or scheduler -- switch again. Previously had NO + * default:/case '?' arm whatsoever. */ + 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, "worker")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "xce/worker>", false, 5000), + "expected to enter the worker context"); + + TEST_ASSERT_EQUAL_INT( + 0, clitest_send_line(&sess, "resupdate -n foo -v 1 --bogus")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "resupdate: unrecognized flag '--bogus'", + true, 5000), + "resupdate must reject an unrecognized flag (Requirement 2.1)"); + + 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'"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, exit_code, + "every rejected invocation above must mark the session failed"); + + clitest_kill_and_reap(&sess); + clitest_close(&sess); + clitest_buf_free(&out); + clitest_buf_free(&err); +} + +/* + * test_walcompare_unrecognized_flag_rejected_with_usage -- covers the ONE + * command that used a fully hand-rolled long-flag-only parser + * (strcmp(argv[ai], "--report")-style) rather than getopt(), per this + * task's explicit scope. Also proves this migration fixed a genuine + * silent-acceptance bug: the pre-migration hand-rolled loop treated ANY + * unrecognized token (including a mistyped "--" flag) as the start of the + * positional member-path list rather than rejecting it, so `walcompare + * --bogus a b` used to run `walcompare` against members ["--bogus", "a", + * "b"] instead of failing loudly. walcompare is a top-level, network-free + * offline tool, so this test needs no context switch and no live + * controller. */ +static void test_walcompare_unrecognized_flag_rejected_with_usage(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, "walcompare --bogus /tmp/a /tmp/b")); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, + "walcompare: unrecognized flag '--bogus'", + true, 5000), + "walcompare must reject an unrecognized long flag instead of " + "silently treating it as a positional member path (Requirement " + "2.1)"); + TEST_ASSERT_TRUE_MESSAGE( + clitest_wait_for_text(&sess, &out, &err, "usage: walcompare", true, + 5000), + "the rejection must include walcompare's usage summary " + "(Requirement 2.4)"); + + 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'"); + TEST_ASSERT_EQUAL_INT_MESSAGE( + 1, exit_code, + "the rejected walcompare invocation must mark the session failed"); + + clitest_kill_and_reap(&sess); + clitest_close(&sess); + clitest_buf_free(&out); + clitest_buf_free(&err); +} + void test_test_cli(void) { UnitySetTestFile(__FILE__); @@ -1082,4 +1442,8 @@ void test_test_cli(void) RUN_TEST(test_source_mutual_cycle_rejected_with_error); RUN_TEST(test_source_missing_file_rejected_with_error); RUN_TEST(test_source_nested_noncyclic_executes_every_line); + RUN_TEST(test_qctrl_missing_action_flag_rejected); + RUN_TEST(test_qctrl_flag_forms_short_and_long_both_accepted); + RUN_TEST(test_migrated_commands_sample_reject_unrecognized_flag); + RUN_TEST(test_walcompare_unrecognized_flag_rejected_with_usage); }