fix(xce-test-cli-ux): route diagnostic/warning messages to stderr uniformly

Audit every printf() call site in test.c not already covered by the
flagspec-rejection or perror() paths; convert 33 genuine error/warning
messages (expect-jobs, expect-job, wait-primary, help's not-found path,
jobrun/jobsub/jobstart/jobfinish/jobfail validation, resupdate, set) from
stdout to stderr, so a command's successful result data and its
diagnostics never share a stream.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019jaMf9QEwpugH2W9KAuc7f
This commit is contained in:
Rongsong Shen 2026-09-10 00:59:38 +00:00
parent 96746af3eb
commit a98fad24da
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
3 changed files with 305 additions and 55 deletions

View file

@ -140,7 +140,7 @@
- _Boundary: Help Renderer_
- _Depends: 1.2, 3.1, 2.6, 3.4_
- [ ] 3.6 Audit and correct stdout/stderr stream routing across existing error paths
- [x] 3.6 Audit and correct stdout/stderr stream routing across existing error paths
- Review every command's remaining error paths not already covered by the shared flag-rejection path built in 1.1 — request failures and internal errors — and ensure diagnostic/error text goes to stderr while a command's successful result data goes to stdout only
- Confirm the same routing holds identically whether stdout and stderr are both attached to a terminal or redirected to separate destinations (a script or pipe)
- Observable completion: redirecting only stdout to a file while triggering a request failure or an internal error on a representative command shows the diagnostic text still appearing on the terminal while the redirected file contains no diagnostic text; separately, running a successful command with only stdout redirected shows its result data captured in the redirected file with nothing sent to stderr
@ -176,6 +176,7 @@
- _Depends: 1.6, 2.3, 3.5, 1.7_
## Implementation Notes
- Audited all 139 `printf(` call sites in test.c; 33 were genuine diagnostic/warning/failure-path messages misrouted to stdout (not covered by the already-correct `flagspec_reject`/`perror` mechanisms) and were converted to `fprintf(stderr, ...)` — concentrated in `cmd_expect_jobs`/`cmd_expect_job`/`cmd_wait_primary` (entirely unrouted; these predate the flagspec migration since they take positional args, not flags), plus `get_setting`, `cmd_help`'s not-found path (a shared `FILE* out` var, stderr only when not-found; bare `help`'s listing and a successful lookup stay stdout), `cmd_ctrlstatus`, `cmd_jobrun`, `cmd_jobsub`, `cmd_jobstart`/`jobfinish`/`jobfail`, `resupdate_report_one`/`cmd_resupdate`, `cmd_set`. Deliberately left as stdout (Requirement 3 doesn't apply): `cmd_resval`'s intentional always-succeeds "not found" design, and the entire async worker/scheduler RPC-handler subsystem (`handle_notify`/`run_job`/etc.) — these are inbound-request callbacks dispatched outside the REPL's `cmd_*` table, not "a command's own diagnostic or result data." Found but explicitly left alone as pre-existing and out of THIS task's scope (stream routing, not return-code correctness): `cmd_resupdate` still unconditionally `return 0`s even on an outright RPC failure or a per-resource rejection — worth a follow-up bug entry if this area is touched again. Requirement 3.4 (redirection-invariance) holds by construction: no code branches a stream CHOICE on `isatty`/`g_interactive` anywhere in the file. (task 3.6)
- `command_t` gained `flagspec`/`flagspec_count`/`usage_tail` fields; `cmd_help` now composes description(`usage` head) -> generated `flagspec_render_help()` block -> `usage_tail` (example/related notes) for all 22 flag-accepting commands, with hand-written per-flag lines deleted from their `USAGE_X`/`USAGE_X_TAIL` constants so the flagspec table is the only place a flag's short/long/required/default lives. Since the four `command_t` tables (`top_commands` etc.) appear before the 22 `flagspec_t` tables in the file, each is forward-declared with an explicit literal size (e.g. `static const flagspec_t jobkill_flags[3];`) rather than the previous `#define X_NFLAGS (sizeof(x_flags)/sizeof(x_flags[0]))` self-computing macro — the literal must now be kept in sync by hand, though any mismatch between the forward declaration and the real definition is a compiler "conflicting types" build error, never a silent drift (review verified this is standard, portable C, and hand-counted several tables' entries against their literals). A follow-up could reorder the flagspec_t tables above the command_t tables to restore the sizeof-based macro and drop this literal-maintenance requirement entirely, but it wasn't required for this task. `join_flags`/`JOIN_FLAGS_COUNT` (a separate translation unit, join_worker_args.c/.h) uses the standard `extern const T arr[N]` header/source split instead, since `extern` against a `static` definition isn't valid — no literal-sync risk there. The five destructive commands' help text now names the confirmation gate and `-y`/`set -y` bypass, scoped accurately (`qctrl`'s note covers only `-i`/`-c`, `hostctrl`'s only `-c`, matching Requirement 9.5's exact enumeration, not their full flag sets). (task 3.5)
- `workerlist`/`joblist`/`reslist` all take the identical long-only `--json` flag (via each command's own `flagspec_t` table); JSON rows emit RAW numeric values for fields the table displays in a humanized/formatted form (`flags` as an int not a hex string, timestamps as raw epoch seconds not `format_job_time`'s human string, `maxmem`/`maxswap`/`maxtmp` as raw bytes not `format_bytes_human`'s string) — deliberate, since Requirement 5.1 asks for "the same fields" (value present) not identical display formatting, and a raw value is strictly more script-consumable. The field-mapping logic was extracted to a new pure module, `listrow_json.c`/`.h` (no RPC/networking/global-state coupling), following the `joblist_display.c` extraction precedent, with full unit coverage in `test_listrow_json.c` (a new standalone `listrow_json_unit_test` meson target, not counted in the main suite's test total) including the fractional-value (`cpufactor` 12.34) real cJSON-print-then-reparse round-trip the task-2.2 note asked for. `joblist_query` gained a 5th `json_mode` parameter; its other two callers (`expect-jobs`/`expect-job`) now pass a literal `0` and are behaviorally unchanged (verified: both `print_table` and `json_mode` false skips both branches exactly as before). Live-cluster E2E visual inspection of real JSON output was not done — same "genuinely untestable without a live cluster" situation task 2.5 already documented for these commands' inlined fetch+print bodies; coverage instead comes from the extracted module's unit tests plus a black-box test proving the flag parses/executes without a flag-parse error. (task 3.4)
- `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)