myxce/.kiro/specs/xce-test-cli-ux/research.md

# Research & Design Decisions

## Summary
- **Feature**: `xce-test-cli-ux`
- **Discovery Scope**: Extension (existing system — `src/controller/testsuite/test.c`, ~4550 lines, the `xce_test` REPL/scripting client)
- **Key Findings**:
  - Every blocking RPC call (`xceapi.c`'s `xce_*` functions) waits on `pthread_cond_timedwait` with a fixed 10s deadline (`cbuf_deadline_10s`); this is **not interrupted by a signal** (glibc's condvar wait retries internally on `EINTR`), so Requirement 1 cannot be satisfied by "check a flag more often" — it needs a client-side detached-worker-thread abandonment pattern, entirely inside `test.c`, never touching `libxce`.
  - Flag parsing today is three uncoordinated patterns (plain `getopt()`, positional-only, one hand-rolled long-flag loop) with **zero shared helper** — every `cmd_*` writes its own `switch`/`default`. 19 short-flag letters exist with heavy collisions (`-m` means 5 different things, `-c` means 6). No fuzzy-match/edit-distance utility exists anywhere in the codebase.
  - `xce_test` already links **cJSON** (via `struct2json`) for an unrelated purpose (job exdata blobs) — this is the natural adopt-not-build choice for Requirement 5's JSON mode. Cap'n Proto is also linked but is wire/blob-only, not a fit for CLI output.
  - `rcfile_load(path, runner)` is already generic enough to be called directly by a new `source` command, but treats a missing file as silent success (wrong for an explicit `source`) and has zero cycle-detection (fine today because it's only ever called once, from a fixed path, at startup).
  - `xce_test`'s own CLI/REPL surface (flag validation, help text, argp, completion) has **zero existing test coverage** — it compiles into the standalone `xce_test` binary, not `controller_unit_test`, so new unit tests need a new `test_test.c`/`test_cli.c` (Unity, `#ifdef XCE_TESTING`-guarded) or black-box subprocess tests matching the existing shell-integration pattern.

## Research Log

### Flag-parsing architecture
- **Context**: Requirements 2 (fail-loud validation), 6 (long-form flags), 10 (flag-suggestion) all require touching every command's flag handling uniformly.
- **Sources Consulted**: `src/controller/testsuite/test.c` (`cmd_jobkill:2567`, `cmd_jobsub:2239-2287`, `cmd_walcompare:1150-1180`, `cmd_setpend:3026-3040`, and 12 other `cmd_*` functions).
- **Findings**:
  - Pattern (a) `getopt()` loop — ~16 commands, each resets `optind = 1`, none use `getopt_long`, `opterr` left at libc default (1), so libc's own diagnostic leaks to stderr inconsistently alongside each command's own (often absent) rejection logic.
  - Pattern (b) positional-only, no dash flags — offline/diagnostic commands (`waldump`, `snapverify`, etc.).
  - Pattern (c) hand-rolled long-flag string-compare loop — `cmd_walcompare` only, fully bespoke.
  - On an unrecognized flag: some commands silently fall through (`default: break;`) and still execute (`jobkill`, `jobckpt`, `qctrl`, `hostctrl`, `runtasks`); some correctly reject with usage + `-EINVAL` (`jobsub`, `resval`, `join`, `jobstart/finish/fail`, `hello`); `setpend` has no `default:` arm at all.
  - No shared usage-printing, rejection, or suggestion helper exists anywhere. The `USAGE_*` string table (`test.c:195-270`) is hand-authored prose, structurally disconnected from the parsing code — this disconnect is the direct root cause of bug-1711-style drift.
  - No Levenshtein/edit-distance utility exists in `src/lib/` or `src/controller/`; a "did you mean" suggestion feature needs a new, small comparison utility.
- **Implications**: A per-command flag-descriptor table (short letter, long name, "concept" name, required, default-description) becomes the single source of truth for (1) building the `getopt_long` `struct option[]`, (2) the shared rejection/suggestion path, and (3) generating the flag-listing portion of `help <cmd>` (Requirement 8.2) — closing the exact drift gap bug-1711 exposed, instead of hand-authoring three parallel copies of the same flag facts.

### Interrupt handling and the REPL loop
- **Context**: Requirement 1 needs Ctrl-C to abandon a blocked RPC wait without waiting for its own timeout.
- **Sources Consulted**: `test.c:657,706,4457` (`sigbreak`/`break_out`), `src/lib/xceapi.c:9,569` (`cbuf_deadline_10s`, `xce_jobctrl`), `src/lib/mylock.c:190` (`cbuf_timedget`), `test.c:13-15,4517-4543` (libedit, main loop), `test.c:3684-3711` (`cmd_set`, `-e`/`+e` toggle).
- **Findings**:
  - `SIGINT` → `sigbreak()` only sets a static `int break_out`; checked in exactly one place (`cmd_hello`'s repeat loop). Every other blocking call ignores it entirely.
  - Every `xce_*` API call in `xceapi.c` (~19 call sites reachable from `cmd_*` handlers) blocks on `pthread_cond_timedwait` with a **fixed 10-second deadline**, via the `cbuf_timedget`/`cbuf_deadline_10s` pair. This primitive is shared library code (`libxce`, `src/lib/`) used by production callers, not just `xce_test`.
  - POSIX/glibc's `pthread_cond_timedwait` does not return early on an asynchronous signal delivered to the same process when only a flag-setting handler is installed — the wait itself is not interruptible this way.
  - `readline()` (libedit, `#include <editline/readline.h>`, confirmed via `meson.build:65`) is used with zero customization (no completion function, no key bindings) — a blank slate for Requirement 7.
  - `cmd_set`'s `-e`/`+e`/`continue-on-error` toggle (`test.c:3697-3711`) is a simple `strcmp` branch flipping one `static` global (`g_continue_on_error`), documented in `USAGE_SET`. All four `command_ctx_t` instances share one static `settings[]` array — settings are session-global, not actually per-context, despite the per-context struct field.
- **Implications**:
  - Requirement 1 requires a **client-side-only** mechanism: run each blocking `xce_*` call on a detached worker thread; have the calling `cmd_*` function poll a short interval (e.g. 50ms) against `break_out`; on interrupt, return immediately without joining, leaving the worker thread to finish or hit its own 10s deadline on its own. This satisfies "leave the connection intact" (1.5) and the Boundary Context's "cancelling a client-side wait does not need to cancel the request server-side" note, and — critically — never touches `libxce`'s shared blocking primitive.
  - A plain blocking `read()`/`fgets()` (for the confirmation prompt, Requirement 9) **is** interrupted by a signal (`EINTR`), unlike the condvar wait — so Ctrl-C-during-confirmation (9.9) is a simpler case than Ctrl-C-during-RPC (1.1) and does not need the same worker-thread pattern.
  - The new confirmation toggle (9.6) should follow the exact same pattern as `-e`/`+e`: one more `static` global flipped by `cmd_set`, documented in `USAGE_SET`.

### rcfile reuse and JSON/serialization availability
- **Context**: Requirement 13 (`source`) and Requirement 5 (JSON output) both ask "is there already a mechanism to adopt?"
- **Sources Consulted**: `src/controller/testsuite/rcfile.c/h`, `test.c:4503-4513`, `src/controller/meson.build:73,77`, `src/meson.build:55`, `test.c:17-18,2195-2202`.
- **Findings**:
  - `rcfile_load(const char* path, int (*runner)(char* line))` is a generic "open, `getline()` loop, call `runner` per line, stop on nonzero" primitive with no rc-file-specific coupling — `source <file>` can call it directly with `preprocess_and_run` as the runner.
  - `rcfile_load` treats a missing file as a silent no-op success (correct for an optional startup rc file, wrong for an explicit user command).
  - No recursion/cycle protection exists anywhere in `rcfile.c` or its call site — a non-issue today only because it's invoked exactly once, from a fixed path. Introducing `source` creates a real cycle path (`source a.txt` inside `a.txt`).
  - cJSON is already linked (`struct2json_dep`, `src/meson.build:55`, `src/controller/meson.build:73`), currently used narrowly for job exdata blobs (`test.c:2195-2202`). No jansson/json-c exists anywhere in the tree.
  - Cap'n Proto is also linked (`c_capnproto_dep`) but exclusively for wire-protocol/checkpoint-blob decoding — not a fit for general CLI text output.
- **Implications**: Adopt cJSON for Requirement 5 (zero new dependency). Build `source` on top of `rcfile_load` directly, adding two things `rcfile.c` doesn't have: an explicit file-not-found error, and cycle detection via a small stack of canonicalized (`realpath`) in-progress source paths, owned by the new `source` command, not by `rcfile.c` itself (which stays untouched and keeps its startup-appropriate silent-missing behavior).

### Steering conventions and test placement
- **Context**: Where do new C files and their tests belong, and what's the C error-handling convention.
- **Sources Consulted**: `.kiro/steering/tech.md`, `structure.md`, `testing.md`, `error-handling.md`; `src/controller/testsuite/` directory listing.
- **Findings**:
  - `gnu17`, Meson, `clang-format`. `testsuite/` is flat — one directory holding both `xce_test`'s own source (`test.c` + a handful of single-purpose helper modules: `rcfile`, `configcompare`, `resval_typeresolve`, `snapdiff`, `snapverify`, `walcompare`, `waldump`) and ~80 Unity `test_*.c` files for the **controller daemon**, unrelated to `xce_test`'s own CLI behavior.
  - TDD is mandatory: a test in `testsuite/` before any implementation change; every new function needs at least one test case.
  - C error convention: negative errno-style return codes (`errnox.h`, `src/include/xce/`), check `rc < 0` immediately, log, propagate.
  - **No existing test targets `test.c`'s own CLI/argp/REPL behavior at all.** New coverage needs a new `test_test.c` (or similarly named) file added to `xce_test_src`/linked with Unity behind `XCE_TESTING`, or black-box subprocess tests in the existing shell-integration style (`clean_failover_preservation.sh`, `test_rcfile_integration.sh`).
- **Implications**: This spec's new modules (flag-descriptor helper, interruptible-call wrapper, JSON list formatter, source command) should each get their own small `test_<module>.c` using Unity, following the existing per-helper-module convention (`test_rcfile.c` next to `rcfile.c`, etc.) rather than growing one monolithic test file. A new black-box CLI test (subprocess-driven) is the right vehicle for cross-cutting behaviors that can't be unit-tested in isolation (e.g., "does `-h` actually print and exit").

## Architecture Pattern Evaluation

| Option | Description | Strengths | Risks / Limitations | Notes |
|--------|-------------|-----------|---------------------|-------|
| Modify `libxce`'s `cbuf_timedget` to poll a cancellation token | Add cooperative cancellation to the shared blocking primitive | Fixes the root cause once, for every caller | Shared code used by production daemons and other clients; changes a core sync primitive for a CLI-UX concern; large blast radius | Rejected — out of boundary |
| Detached worker thread + poll `break_out`, abandon on interrupt | Run the existing blocking call unchanged on a worker thread; main thread polls a short interval | Zero changes to `libxce`; contained entirely in `test.c`; matches existing "cancel client-side wait, not the server request" expectation | Requires careful heap-ownership handoff so an abandoned thread never touches caller stack memory after the caller returns | **Selected** |
| `pthread_cancel()` the blocking thread | Forcibly cancel the worker thread at a cancellation point | Simpler poll-free code | `pthread_cond_timedwait` cancellation can leave `libxce`'s internal mutex/state inconsistent (async-cancel-safety not established for that code); much higher risk | Rejected |
| getopt_long + shared flag-descriptor table | One small table per command drives long options, rejection, suggestion, and help-text generation | Kills the bug-1711 drift class at the root; one utility instead of three divergent patterns | Touches every flag-parsing `cmd_*` function (~17) | **Selected** |
| Keep three existing parsing patterns, bolt on rejection/suggestion per-command | Minimal structural change | Less code churn up front | Perpetuates the exact inconsistency this spec exists to fix; would need the same fixes copy-pasted N times | Rejected |
| Adopt cJSON for Requirement 5 | Use the already-linked JSON library | Zero new dependency, already proven in this binary | None material | **Selected** |
| Hand-roll a minimal JSON writer | Avoid any library dependency | N/A — cJSON is already linked | Pure waste, reinvents what's already available | Rejected |

## Design Decisions

### Decision: Interruptible RPC via detached worker thread with CAS-based ownership handoff
- **Context**: Requirement 1 demands Ctrl-C abandon a blocked RPC wait immediately; the underlying wait primitive is not signal-interruptible and lives in shared library code out of this spec's boundary.
- **Alternatives Considered**: modify `libxce` (rejected, out of boundary); `pthread_cancel` (rejected, cancellation-safety unproven for the target code).
- **Selected Approach**: Each blocking `xce_*` call site is wrapped by a shared `xcetest_run_interruptible()` helper. The call context is heap-allocated. A worker thread runs the real blocking call and, on completion, atomically transitions a shared state word from `RUNNING` to `DONE_UNCLAIMED`; if that CAS fails (because the poller already flipped it to `ABANDONED`), the worker frees the block itself. The polling caller loops on a short sleep checking `break_out`; on interrupt it CASes `RUNNING` to `ABANDONED` — if that succeeds, it returns `-EINTR` immediately without touching the block again (the worker will free it later); if it fails (the worker already finished), it falls through to the normal completion path.
- **Rationale**: Contained entirely within `test.c`/testsuite; no shared-library change; no dangling-stack-frame risk because ownership of the heap block is resolved by a single atomic CAS regardless of which side "wins" the race.
- **Trade-offs**: A worker thread can outlive the `cmd_*` call that spawned it by up to ~10s after an abandonment; this is intentional and matches the existing boundary note that a client-side cancel need not cancel the server-side request.
- **Follow-up (resolved during design review)**: The original follow-up here — "verify no `xce_*` call site holds a non-reentrant lock across the call that a second, overlapping invocation could contend" — understated the actual risk: every connection has genuinely shared, non-reentrant state (its single-outstanding-reply buffer), so an abandoned worker and a newly issued call on the *same* connection racing each other is not a rare edge case but the direct, likely consequence of an operator doing exactly what Ctrl-C is for (cancel, then immediately try something else). See "Decision: Per-connection lock serializes overlapping calls" below for the fix folded into the same component.

### Decision: Per-connection lock serializes overlapping calls on the Interruptible Call Adapter
- **Context**: Found during the design review gate (kiro-validate-design), not during initial discovery — an abandoned worker thread keeps running against the same connection object every subsequent command on that context also uses; a second call issued right after an abandonment would race the first for that connection's single-outstanding-reply state, with no synchronization.
- **Alternatives Considered**: leave it undocumented as an accepted risk (rejected — silently accepting a footgun that undermines the very safety property Requirement 1 exists to provide is not an acceptable simplification); have each `cmd_*` function track its own "is a prior call on this connection still outstanding" flag (rejected — duplicates the same bookkeeping the adapter already needs to do once, centrally).
- **Selected Approach**: `xcetest_run_interruptible` acquires a per-connection lock (keyed by the currently active `command_ctx_t`'s connection pointer, in a small lazily-populated registry) before spawning the worker, and holds it until the worker actually finishes — including through an abandonment. Acquiring the lock is itself interruptible (the same `break_out`-polling loop), so a second command on the same connection can still be Ctrl-C'd out of its own wait for the lock.
- **Rationale**: Bounds the unsafe-overlap window to zero rather than ~10s; contained entirely within the same component already responsible for this call's lifecycle; no new dependency.
- **Trade-offs**: A second command on the same connection issued immediately after an abandonment now waits (interruptibly) for the abandoned worker to finish, rather than proceeding immediately — an intentional, bounded cost in exchange for correctness.

### Decision: Confirmation prompt's Ctrl-C-as-decline requires SA_RESTART cleared on SIGINT
- **Context**: Found during the design review gate — the Confirmation Gate's Requirement 9.9 assumed a blocking `read()` is reliably interrupted by `SIGINT` (`EINTR`), but the existing `signal(SIGINT, sigbreak)` installation (`test.c:4457`) uses `signal()`'s BSD semantics, which historically restart an interrupted slow syscall by default unless `SA_RESTART` is explicitly cleared.
- **Alternatives Considered**: assume the existing installation already behaves correctly (rejected — exactly the kind of unverified assumption the RPC-wait investigation already showed is unsafe in this codebase); install a second, confirm.c-local signal handler only around the prompt read (rejected — more moving parts than fixing the one global installation, and risks the two handlers's dispositions getting out of sync).
- **Selected Approach**: Replace the existing `signal(SIGINT, sigbreak)` call in `main()` with an explicit `sigaction()` call with `SA_RESTART` cleared, once, at startup. Verify at implementation time (a small standalone test: send SIGINT during a blocking `read()` and confirm `EINTR`, not a silent retry).
- **Rationale**: A single, global, one-time fix; does not affect the Interruptible Call Adapter, which never relies on `EINTR` (it polls `break_out` on a timer regardless of restart semantics).
- **Trade-offs**: None material.

### Decision: Flag-descriptor table as the single source of truth for parsing, validation, and help text
- **Context**: Requirements 2, 6, 8.2, and 10 all touch the same underlying fact (what flags a command has, their short/long form, required-ness, default) from four different angles.
- **Alternatives Considered**: hand-maintain four separate representations (existing state, rejected — this is the mechanism that produced bug-1711); fully generate `help <cmd>`'s entire text from the table (rejected — the existing convention's hand-authored prose/examples/related-command notes carry information a table can't express, and Requirement 8's prose/example/related-command ACs are inherently free-text).
- **Selected Approach**: A small `flagspec_t[]` array per command (short letter, long name, concept name, required, default description) builds the `getopt_long` option table, drives the shared rejection/suggestion helper, and generates only the flag-listing portion of `help <cmd>` — the hand-authored `USAGE_*` prose keeps its description/example/related-command sections.
- **Rationale**: Closes the drift gap at its root while respecting the existing, otherwise-working hand-authored-help convention (`test.c:195-205`).
- **Trade-offs**: Every flag-parsing `cmd_*` function's `getopt()` call becomes `getopt_long()` plus a small static table — mechanical, but touches ~17 functions.

### Decision: `source` built directly on `rcfile_load`, cycle detection owned by the new command
- **Context**: Requirement 13 needs file-execution logic that already exists in `rcfile_load`, but that function's missing-file and cycle behavior are wrong for an explicit user command.
- **Alternatives Considered**: modify `rcfile_load` itself to add cycle detection and a caller-selectable missing-file policy (rejected — would change behavior for the startup rc-file path too, an unrelated concern, and mixes two callers' needs into one function); duplicate the file-reading loop in a new function (rejected — pure duplication of working code).
- **Selected Approach**: `cmd_source` calls `access(path, R_OK)` itself before calling `rcfile_load(path, preprocess_and_run)` unchanged, and maintains its own small static stack of canonicalized (`realpath`) in-flight source paths, pushed/popped around the `rcfile_load` call, checked for membership before push.
- **Rationale**: `rcfile.c` stays untouched (no risk to the already-shipped `xce-test-rc-file` behavior); the new command owns the two behaviors specific to its own use case.
- **Trade-offs**: None material — `rcfile_load`'s reuse is a pure win here.

## Risks & Mitigations
- **libedit's completion API may not perfectly mirror GNU readline's `rl_attempted_completion_function`** — verify at implementation start; libedit has historically tracked the readline completion API closely, but this project's linked version should be confirmed before committing to the exact signature in code.
- **Migrating ~17 `cmd_*` functions from `getopt()` to `getopt_long()` touches a large, currently-untested surface** — mitigated by the new `test_test.c`/black-box CLI test suite being written test-first per this project's TDD mandate, one command at a time.
- **The detached-worker-thread pattern introduces xce_test's first internal use of `pthread_create`** — confirm `pthread` is already linked (transitively via `libxce`, which itself is threaded) rather than assuming; a quick `meson.build` check at implementation start avoids a surprise link error.
- **Confirmation-prompt reads must not be routed through libedit's `readline()`** (that would pollute history with y/n answers and behave inconsistently under non-TTY) — use a plain blocking read instead, made reliably interruptible by `EINTR` via the `sigaction()`/`SA_RESTART` fix (see Design Decisions).
- **`cmd_qctrl`'s existing uninitialized `action` variable (bug-1704)** — surfaced while writing the Flag Parsing Layer's migration note for `cmd_qctrl`; the Confirmation Gate's Requirement 9.5 depends on reliably knowing whether `-i`/`-c` was given, which is unsafe against an uninitialized value. Mitigated by applying `cmd_hostctrl`'s existing sentinel-and-reject pattern to `cmd_qctrl` as part of its flagspec migration (see design.md, Flag Parsing Layer implementation notes).

## References
- `src/controller/testsuite/test.c` — the file under change throughout.
- `src/lib/xceapi.c`, `src/lib/mylock.c` — blocking RPC primitive (read-only reference, out of boundary for modification).
- `src/controller/testsuite/rcfile.c/h` — reused as-is by `source`.
- `.kiro/specs/xce-test-rc-file/` — prior, already-shipped spec that built `rcfile.c`.
- `.wolf/buglog.json` bug-1711, bug-1557 — the two bugs folded into this spec's Requirements 11 and 12.
- `.wolf/buglog.json` bug-1704 — `cmd_qctrl`'s uninitialized `action` variable, found during design review; fixed as a side effect of the Flag Parsing Layer's migration of `cmd_qctrl` (not a formal requirements.md addition, since it falls out of the same work Requirement 2/9.5 already demand).