docs(xce-test-cli-ux): commit requirements/design/tasks spec-phase artifacts
Captures the spec-authoring work from earlier in this session: expanded
requirements.md (Requirements 8-13 added during discussion: detailed
help content, destructive-command confirmation, actionable flag
suggestions, bug-1711/bug-1557 fixes, and the source command), plus
the corresponding spec.json phase/approval updates. design.md and
tasks.md were already committed alongside the first implementation
task (0748f10c); this closes the gap so the full spec-authoring
history is captured in version control, matching the approvals the
user already gave interactively during this session's requirements
review, design review, and task-plan generation.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019jaMf9QEwpugH2W9KAuc7f
This commit is contained in:
parent
4d1f247674
commit
7d0ac6f6eb
3 changed files with 212 additions and 24 deletions
|
|
@ -6,9 +6,9 @@
|
|||
|
||||
## Boundary Context
|
||||
|
||||
- **In scope**: process-level `-h`/`--help`/`--version` behavior; uniform rejection of unrecognized or malformed command-line flags across every `xce_test` command; consistent routing of error/diagnostic text to stderr and command-result data to stdout; Ctrl-C (SIGINT) behavior that cancels the in-flight operation or the current input line instead of silently doing nothing or exiting the session; a machine-readable output mode for the tabular listing commands (`workerlist`, `joblist`, `reslist`); command-not-found guidance that tells the user when a command exists in a different context; adding a long-form flag equivalent, consistently named per concept, for every command's existing short-only flags; command-name tab completion at the interactive prompt.
|
||||
- **Out of scope**: the `xce_test` → `xce_harness`-style rename (tracked separately, to be done later); any change to controller/worker/scheduler wire protocol or RPC semantics; the auto-variable, exit-code-verdict, or shell-escape (`!`) mechanisms already shipped by `xce-test-e2e-primitives`; a full curses/TUI mode; color/theming; a `.xce_testrc` auto-executed startup-file feature (tracked as a separate, later task); changing what a successful command's result data actually contains; **renaming, removing, or changing the meaning of any existing short flag** (e.g. resolving `-m`'s four different meanings, or `-c`'s three, by reassigning a letter) — that is a breaking change to every script already using these flags and is deferred to a future, explicitly-scoped decision, not bundled into this feature; tab completion of flag *values* (jobids, hostnames, resource names) — only command-*name* completion is in scope; a per-command `-h`/`--help` flag handled by each `cmd_*` function individually — the existing in-session `help <cmd>` command already serves this need and is not being duplicated.
|
||||
- **Adjacent expectations**: `xce-test-e2e-primitives` explicitly deferred structured/JSON output as out of its boundary — this feature now owns that gap for the listing commands only (`jobsub`/`joblist`'s existing `$jobid`/`$jobcount` auto-variable capture is unaffected and remains the scripted way to read those specific values). Cancelling a client-side wait on Ctrl-C does not need to cancel the request on the controller/worker/scheduler side — the daemon's own RPC handling and timeouts are unaffected. Existing scripts that parse the current human-readable table output must keep working unchanged when the new machine-readable mode is not requested. Requirement 4's process-level `-h`/`--help` and the existing in-session `help <cmd>` command remain the sole help surfaces — Requirement 7's long-form flags are additive documentation of an existing command's own flags, not a new help mechanism.
|
||||
- **In scope**: process-level `-h`/`--help`/`--version` behavior; uniform rejection of unrecognized or malformed command-line flags across every `xce_test` command; consistent routing of error/diagnostic text to stderr and command-result data to stdout; Ctrl-C (SIGINT) behavior that cancels the in-flight operation or the current input line instead of silently doing nothing or exiting the session; a machine-readable output mode for the tabular listing commands (`workerlist`, `joblist`, `reslist`); adding a long-form flag equivalent, consistently named per concept, for every command's existing short-only flags; command-name tab completion at the interactive prompt; detailed, consistent per-command help content (purpose description, full flag documentation, an example, and related-command/context notes) in `help <cmd>` output; a confirmation prompt, bypassable via a per-command flag, a session-scoped confirmation toggle, or non-interactive mode, before executing a destructive command; suggesting the likely-intended flag when an unrecognized flag closely matches exactly one flag the command accepts; correcting bug-1711's display defects — the "UNKOWN"/"matchs"/"sequentail" text typos, `reslist`'s inconsistent column justification and unlabeled hex Flags column, `joblist`'s raw-epoch timestamps and its mintasks/maxtasks pair mislabeled as a single "NTasks" column, `joblist`'s missing raw-state fallback for an ambiguous RUN+EXIT state, and `getconf`'s silently-empty group/config sections; correcting `join`'s stale byte-scale `maxmem` default (bug-1557) so its documented default in help text is also a correct one; an in-session `source <file>` command for composing reusable command sequences from files, including nested sourcing with cycle detection.
|
||||
- **Out of scope**: the `xce_test` → `xce_harness`-style rename (tracked separately, to be done later); any change to controller/worker/scheduler wire protocol or RPC semantics; the auto-variable, exit-code-verdict, or shell-escape (`!`) mechanisms already shipped by `xce-test-e2e-primitives`; a full curses/TUI mode; color/theming; a `.xce_testrc` auto-executed startup-file feature (tracked as a separate, later task); changing what a successful command's result data actually contains; **renaming, removing, or changing the meaning of any existing short flag** (e.g. resolving `-m`'s four different meanings, or `-c`'s three, by reassigning a letter) — that is a breaking change to every script already using these flags and is deferred to a future, explicitly-scoped decision, not bundled into this feature; tab completion of flag *values* (jobids, hostnames, resource names) — only command-*name* completion is in scope; a per-command `-h`/`--help` flag handled by each `cmd_*` function individually — the existing in-session `help <cmd>` command already serves this need and is not being duplicated; a full error-code taxonomy with reference URLs — this tool has no external documentation site to link to, so actionable error output is limited to a same-command flag-name suggestion; a persistent, configurable default for skipping destructive-command confirmation that survives across separate xce_test invocations (e.g. a config file or environment variable) — Requirement 9's bypass mechanisms (a per-command flag and a session-scoped confirmation toggle) both reset to confirmation-on at the start of every new xce_test process; **context-aware command-not-found guidance** — telling the user that an unrecognized command exists in a different context (considered, then deliberately dropped): both command dispatch and `help <cmd>` lookups are scoped to the active context only, and an unrecognized name is reported as not found there without checking any other context.
|
||||
- **Adjacent expectations**: `xce-test-e2e-primitives` explicitly deferred structured/JSON output as out of its boundary — this feature now owns that gap for the listing commands only (`jobsub`/`joblist`'s existing `$jobid`/`$jobcount` auto-variable capture is unaffected and remains the scripted way to read those specific values). Cancelling a client-side wait on Ctrl-C does not need to cancel the request on the controller/worker/scheduler side — the daemon's own RPC handling and timeouts are unaffected. Existing scripts that parse the current human-readable table output must keep working unchanged when the new machine-readable mode is not requested. Requirement 4's process-level `-h`/`--help` and the existing in-session `help <cmd>` command remain the sole help surfaces — Requirement 6's long-form flags and Requirement 8's expanded content are additive documentation of an existing command's own flags and behavior, not a new help mechanism. Requirement 9's session-scoped confirmation toggle (9.6-9.7) is how a `source`d file (Requirement 13) runs destructive commands unattended without per-command bypass flags — a script author sets the toggle explicitly before or within the sourced file, rather than `source` implicitly granting or blocking confirmation bypass on its own.
|
||||
|
||||
## Requirements
|
||||
|
||||
|
|
@ -19,7 +19,7 @@
|
|||
#### Acceptance Criteria
|
||||
1. When the operator presses Ctrl-C while a command is blocked awaiting a cluster RPC reply, the xce_test session shall abandon the wait immediately, without waiting for the RPC's own timeout to elapse, and return control to the prompt.
|
||||
2. When the operator presses Ctrl-C at the command prompt with no command in flight, the xce_test session shall discard the currently-typed input line and redisplay the prompt.
|
||||
3. If a command is interrupted by Ctrl-C, then the xce_test session shall report that command as failed, consistent with how any other failing command is reported.
|
||||
3. If a command is interrupted by Ctrl-C, then the xce_test session shall report that command as failed, consistent with how any other failing command is reported, except a destructive command's confirmation prompt (Requirement 9.9), which is reported as not run rather than failed.
|
||||
4. The xce_test session shall exit only as a direct result of an explicit `quit` command, an EOF at the top-level prompt, or an abort-on-failure condition in scripted mode — never as a side effect of a single Ctrl-C keypress.
|
||||
5. While a command is being interrupted by Ctrl-C, the xce_test session shall leave the active worker/scheduler/client context and its underlying connection intact.
|
||||
|
||||
|
|
@ -58,20 +58,11 @@
|
|||
|
||||
#### Acceptance Criteria
|
||||
1. Where a machine-readable output mode is requested for `workerlist`, `joblist`, or `reslist`, the xce_test session shall emit each record in a structured, parseable format (for example, JSON) containing the same fields as that command's human-readable table.
|
||||
2. When no machine-readable output mode is requested, the xce_test session shall continue to print the existing human-readable table for `workerlist`, `joblist`, and `reslist`, unchanged.
|
||||
2. When no machine-readable output mode is requested, the xce_test session shall continue to print the existing human-readable table for `workerlist`, `joblist`, and `reslist`, unchanged, except where Requirement 11 supersedes this Acceptance Criterion for a specific defect it corrects (`reslist`'s column justification and Flags formatting; `joblist`'s timestamp formatting, NTasks column, and state fallback).
|
||||
3. The xce_test session shall not truncate or misalign a field's value in machine-readable output regardless of the field value's length.
|
||||
4. The xce_test session shall document, via `help <cmd>`, how to request the machine-readable output mode for each command that supports it.
|
||||
|
||||
### Requirement 6: Context-Aware Command-Not-Found Guidance
|
||||
|
||||
**Objective:** As a user new to `xce_test`'s top/worker/scheduler/client context tree, I want an unrecognized command to tell me if it exists in a different context, so that I can find where to run it without trial and error.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. When a typed command is not found in the active context, and a command of that exact name exists in a different context, the xce_test session shall report the command as not found in the current context and identify the context where it does exist.
|
||||
2. When a typed command is not found in the active context and does not exist in any context, the xce_test session shall report it as not found without naming an unrelated context.
|
||||
3. The xce_test session shall not change the active context or execute any command as a result of a not-found lookup.
|
||||
|
||||
### Requirement 7: Long-Form, Cross-Command-Consistent Flags
|
||||
### Requirement 6: Long-Form, Cross-Command-Consistent Flags
|
||||
|
||||
**Objective:** As a user or script author who has learned one `xce_test` command's flags, I want a long-form equivalent of every flag, named the same way for the same concept across every command that accepts it, so that muscle memory from one command doesn't mislead me in another and my scripts can be self-documenting.
|
||||
|
||||
|
|
@ -81,7 +72,7 @@
|
|||
3. When a long-form flag equivalent is added for an existing short flag, the xce_test session shall leave that short flag's accepted values, default, and behavior unchanged.
|
||||
4. The xce_test process's help output for a command (`help <cmd>`) shall list both the short and long form of each of that command's flags.
|
||||
|
||||
### Requirement 8: Command-Name Tab Completion
|
||||
### Requirement 7: Command-Name Tab Completion
|
||||
|
||||
**Objective:** As an interactive user, I want tab completion for command names at the prompt, so that I can discover and enter commands without memorizing the full command table or re-running `help`.
|
||||
|
||||
|
|
@ -90,3 +81,75 @@
|
|||
2. When the partially-typed text unambiguously matches exactly one command name in the active context, pressing tab shall complete it in full.
|
||||
3. When the partially-typed text matches more than one command name in the active context, pressing tab shall list the matching candidates without altering the input line.
|
||||
4. The xce_test session shall not offer a command name as a completion candidate when that command is not valid in the currently active context.
|
||||
|
||||
### Requirement 8: Detailed, Consistent Per-Command Help Content
|
||||
|
||||
**Objective:** As a user or script author running `help <cmd>`, I want every command's help text to consistently include what the command does, every flag it accepts, an example, and how it relates to other commands, so that I don't have to read the source to use it correctly.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. The xce_test session's `help <cmd>` output shall include a one-to-two sentence description of the command's purpose, in addition to its usage syntax line.
|
||||
2. For every flag a command accepts, the xce_test session's `help <cmd>` output shall state both the short and long form of that flag (per Requirement 6), whether it is required, and its default value if one exists.
|
||||
3. The xce_test session's `help <cmd>` output shall include at least one concrete example invocation for every command that accepts flags or arguments.
|
||||
4. Where invoking a command sets scripting state read elsewhere (for example, an auto-variable) or the command is commonly paired with another command, the xce_test session's `help <cmd>` output shall name that related state or command.
|
||||
5. The xce_test session shall apply Acceptance Criteria 1-4 uniformly to every command's help text — no command's help text is exempt.
|
||||
6. Where a command's behavior, output, or the entity it acts on differs depending on the active context (for example, which entity's data a shared command like `waldump` reads), the xce_test session's `help <cmd>` output shall describe the behavior specific to the currently active context, not a generic description that omits the context-specific difference.
|
||||
7. When `help <cmd>` is given a name that is not a command in the active context, the xce_test session shall report it as not found in the active context without checking whether that name exists as a command in a different context.
|
||||
|
||||
### Requirement 9: Confirmation for Destructive Commands
|
||||
|
||||
**Objective:** As an operator running xce_test interactively against a live or degraded HA cluster, I want a destructive command to ask for confirmation before it executes, so that a typo or a misremembered jobid doesn't take an unintended, irreversible action on the cluster.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. When a destructive command is invoked interactively without an explicit bypass flag, the xce_test session shall prompt the operator to confirm before executing it.
|
||||
2. When the operator declines the confirmation prompt, the xce_test session shall abort the command without executing it and report it as not run, distinct from reporting it as failed.
|
||||
3. The xce_test session shall provide a long-form bypass flag (for example, `--yes`) on every destructive command to skip the confirmation prompt.
|
||||
4. When xce_test's input is not from an interactive TTY, or the bypass flag is given, the xce_test session shall execute the destructive command without prompting.
|
||||
5. The xce_test session shall treat the following, and only the following, as destructive commands subject to Acceptance Criteria 1-4 and 6-9: `jobkill` (client context); `shutdown` (client context); `qctrl` invoked with `-i` or `-c` (client context); `hostctrl` invoked with `-c` (client context); and `upgradeconfig` (client context). `qctrl` invoked with `-a`/`-o` and `hostctrl` invoked with `-o` are reversal/reactivation actions and shall not require confirmation. `jstart`/`jfinish`/`jfail` (worker context) are excluded — they simulate a fake worker's own job-lifecycle facts for test purposes and do not warrant a confirmation prompt. This determination applies identically regardless of whether the qualifying flag (`-i`/`-c`/`-a`/`-o`) is given in its short or long form (Requirement 6).
|
||||
6. The xce_test session shall provide a session-scoped confirmation toggle, in the same style as its existing `-e`/`+e`/continue-on-error toggle, defaulting to confirmation-on.
|
||||
7. While the session-scoped confirmation toggle is set to skip confirmation, the xce_test session shall execute every destructive command without prompting, regardless of TTY state, until the toggle is set back to confirmation-on — so that a script, including one run via `source` (Requirement 13), can explicitly opt in to unattended execution of destructive commands without needing a per-command bypass flag on each one.
|
||||
8. The xce_test session shall write the confirmation prompt text to stderr, consistent with Requirement 3's routing of non-result text.
|
||||
9. When the operator presses Ctrl-C while a confirmation prompt is displayed, the xce_test session shall treat it the same as declining the prompt (Acceptance Criterion 2), shall not execute the destructive command, and shall report it as not run rather than failed — a deliberate exception to Requirement 1.3's general Ctrl-C-as-failure rule, since declining a destructive action is a safe outcome, not an error.
|
||||
|
||||
### Requirement 10: Actionable Error Messages for Invalid Flags
|
||||
|
||||
**Objective:** As a user or script author who mistypes a flag, I want the rejection message to suggest the flag I probably meant, so that I can fix the invocation without re-reading the command's full help text.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. When an unrecognized flag closely matches exactly one flag the command accepts (for example, a common misspelling or that flag's short/long counterpart), the xce_test session shall name that flag as the likely intended one in its rejection message, in addition to the usage summary already required by Requirement 2.4.
|
||||
2. When an unrecognized flag does not closely match any flag the command accepts, the xce_test session shall report it as unrecognized without guessing, consistent with Requirement 2.1.
|
||||
3. The xce_test session shall apply Acceptance Criteria 1-2 uniformly across every command that accepts flags.
|
||||
|
||||
### Requirement 11: Correctness Fixes for Existing Output and Help-Text Defects (bug-1711)
|
||||
|
||||
**Objective:** As a user of xce_test's listing commands and help text, I want its existing display defects — typos, an inconsistent table format, silently-empty sections, and a missing state-display fallback — fixed, so that its output is trustworthy and internally consistent instead of accumulating small surprises.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. The xce_test session shall report an unrecognized worker status bit using correctly spelled text ("UNKNOWN"), not "UNKOWN".
|
||||
2. When `help <cmd>` is given an unknown topic, the xce_test session shall report it using grammatically correct text ("No commands match `%s'"), not "No commands matchs `%s'" — this is a text-grammar fix only and does not add the cross-context lookup excluded by Requirement 8.7.
|
||||
3. The xce_test session's `runtasks` command shall label its default single-task launch type as "sequential", not "sequentail", wherever that value is displayed.
|
||||
4. The xce_test session's `reslist` command shall left-justify its Name and Description text columns, consistent with `workerlist`'s equivalent columns, and shall label and prefix its Flags column as hexadecimal (for example, `0x%x`), consistent with this file's `state_raw` convention.
|
||||
5. The xce_test session's `joblist` command shall format its SubmitTime, StartTime, and FinishTime columns using this file's existing human-readable time formatting (as `ctrlstatus` already does), not raw Unix epoch seconds.
|
||||
6. The xce_test session's `joblist` command shall present its hidden mintasks/maxtasks pair as two distinct, correctly labeled columns rather than a single ambiguous "NTasks" column.
|
||||
7. The xce_test session's `joblist` command shall report an ambiguous RUN+EXIT job state using a raw-state fallback, consistent with the fallback `print_jobinfo` already applies for that same ambiguity.
|
||||
8. The xce_test session's `getconf` command shall either print the actual contents of its User Groups, Host Groups, and Configs sections, or, if a section is not yet implemented, print an explicit "(not implemented)" marker — never a header followed by silent nothing that is indistinguishable from a genuinely empty cluster.
|
||||
|
||||
### Requirement 12: Correct join's Stale maxmem Default (bug-1557)
|
||||
|
||||
**Objective:** As an operator using `join` to simulate a worker joining without a real `xcecd`, I want its default `maxmem` value to represent a sane amount of memory in the codebase's current bytes-canonical unit, so that a worker joined without `-m` doesn't silently report 64 bytes of RAM.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. When the `join` command is invoked without a maxmem flag, the xce_test session shall default the simulated worker's maxmem to 64 * 1024 * 1024 (64 MB expressed in bytes), consistent with this codebase's bytes-canonical convention for maxmem.
|
||||
2. The xce_test session's help text for `join` shall document this corrected default value, consistent with Requirement 8.2's requirement to state each flag's default.
|
||||
|
||||
### Requirement 13: In-Session Script Sourcing
|
||||
|
||||
**Objective:** As an interactive or scripted xce_test user, I want a `source <file>` command that reads and executes another file's commands within the current session, so that I can compose reusable command sequences without restarting the process or resorting to shell-level concatenation.
|
||||
|
||||
#### Acceptance Criteria
|
||||
1. When the user runs `source <file>`, the xce_test session shall read and execute each line of that file as if it had been typed at the current prompt, in order.
|
||||
2. When `source <file>` is invoked and the file does not exist or cannot be read, the xce_test session shall report the error and shall not treat any line of that file as executed.
|
||||
3. When a command sourced from a file fails, the xce_test session shall apply the same abort-on-failure/continue-on-error behavior (per the session's current `-e`/`+e`/`continue-on-error` setting) to that file's remaining lines as it applies to the top-level input stream.
|
||||
4. The xce_test session shall support invoking `source` from within a sourced file, so that one script file can compose others.
|
||||
5. When a `source` command would create a cycle (a file sourcing itself, directly or transitively), the xce_test session shall detect this, reject the command, and report an error, instead of hanging or exhausting resources.
|
||||
6. The xce_test session shall document `source`'s usage, behavior, and interaction with the continue-on-error/abort-on-failure setting in `help source`, consistent with Requirement 8.
|
||||
7. The xce_test session shall evaluate Requirement 9's TTY-based confirmation-prompt condition (Acceptance Criterion 4) against the top-level session's own standard input, not against whether a command's text currently originates from a sourced file — so a destructive command inside a `source`d file in an otherwise-interactive session still prompts for confirmation unless the session-scoped confirmation toggle (Requirement 9.6-9.7) is set to skip it.
|
||||
|
|
|
|||
125
.kiro/specs/xce-test-cli-ux/research.md
Normal file
125
.kiro/specs/xce-test-cli-ux/research.md
Normal file
|
|
@ -0,0 +1,125 @@
|
|||
# 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).
|
||||
|
|
@ -1,22 +1,22 @@
|
|||
{
|
||||
"feature_name": "xce-test-cli-ux",
|
||||
"created_at": "2026-08-26T08:38:15Z",
|
||||
"updated_at": "2026-08-26T09:44:21Z",
|
||||
"updated_at": "2026-09-08T22:15:00Z",
|
||||
"language": "en",
|
||||
"phase": "requirements-generated",
|
||||
"phase": "tasks-generated",
|
||||
"approvals": {
|
||||
"requirements": {
|
||||
"generated": true,
|
||||
"approved": false
|
||||
"approved": true
|
||||
},
|
||||
"design": {
|
||||
"generated": false,
|
||||
"approved": false
|
||||
"generated": true,
|
||||
"approved": true
|
||||
},
|
||||
"tasks": {
|
||||
"generated": false,
|
||||
"approved": false
|
||||
"generated": true,
|
||||
"approved": true
|
||||
}
|
||||
},
|
||||
"ready_for_implementation": false
|
||||
"ready_for_implementation": true
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue