feat(xce-test-cli-ux): wire confirmation gate to the five destructive commands

Gates jobkill and shutdown unconditionally, qctrl only when action is
QUEUE_CLOSE/QUEUE_INACTIVE (not ACTIVE/OPEN), hostctrl only when
HOST_CLOSE (not OPEN), and upgradeconfig unconditionally -- exactly
Requirement 9.5's enumeration, with jstart/jfinish/jfail (worker
context) correctly untouched. Each gains a -y/--yes bypass flag via
flagspec_t; cmd_set gains -y/+y calling confirm_set_skip(). A decline
maps to a plain `return 0` (success-coded, distinct from a negative
failure return) per confirm.h's own documented contract -- this means
a declined destructive command inside a source'd file under `set -e`
won't trip abort-on-failure, a real but narrow interaction only
reachable when a human is actively declining; left for task 4.3 to
test explicitly. 1321/1321 across the full controller suite (+3),
0 regressions.

Task 3.3 of .kiro/specs/xce-test-cli-ux/tasks.md.
Requirements: 9.1, 9.2, 9.3, 9.4, 9.5, 9.6, 9.7, 9.8, 9.9

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-09 21:47:55 +00:00
parent 45120af2cc
commit a57c04716b
No known key found for this signature in database
GPG key ID: 5BCDAAE0123E9FE2
3 changed files with 635 additions and 5 deletions

View file

@ -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_<module>(void);` + `TEST(test_<module>);` 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 <file with a declined destructive command>` 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)