Expand unit test coverage and add TTY test infrastructure

Author: umair-ably

.claude/skills/ably-codebase-review/SKILL.md

@@ -154,14 +154,21 @@ Launch these agents **in parallel**. Each agent gets a focused mandate and uses
 
 **Method (grep/glob — text patterns and file matching):**
 1. **Glob** for each command in `src/commands/` and check if a corresponding test file exists at `test/unit/commands/`
-2. **Grep** for `describe(` in test files to check for required describe blocks: `help`, `argument validation`, `functionality`, `flags`, `error handling`
-3. **Grep** for `--duration` in subscribe test files
+2. **Grep** for `describe(` in test files to check for the 5 required describe blocks with EXACT standard names:
+   - `describe("help"` — required in every test file
+   - `describe("argument validation"` — required (test required args OR unknown flag rejection)
+   - `describe("functionality"` — required (core happy-path tests)
+   - `describe("flags"` — required (verify flags exist and work)
+   - `describe("error handling"` — required (API errors, network failures)
+   Flag non-standard variants: `"command arguments and flags"`, `"command flags"`, `"flag options"`, `"parameter validation"`.
+   Exception: `interactive.test.ts`, `interactive-sigint.test.ts`, and `bench/*.test.ts` are exempt.
+3. **Grep** for `--duration` in unit test `runCommand()` args — should NOT be present (env var handles it). Exceptions: `test:wait` tests, `interactive-sigint` tests, help output checks.
 4. **Grep** for `getMockAblyRealtime`, `getMockAblyRest`, `getMockConfigManager` in test files to verify correct mock usage per command type
 5. **Grep** for `--api-key`, `--token`, `--access-token` in unit test files — these should not use CLI auth flags
 
 **Reasoning guidance:**
 - Missing test files are deviations but may be documented as known gaps
-- Missing describe blocks in existing tests are deviations but may be lower priority
+- Missing describe blocks or non-standard block names are deviations that should be flagged
 - Subscribe tests that auto-exit via mocked callbacks (without `--duration`) may be acceptable
 
 ### Agent 7: Lifecycle & Convention Sweep

.claude/skills/ably-new-command/SKILL.md

@@ -363,7 +363,8 @@ If the new command shouldn't be available in the web CLI, add it to the appropri
 
 After creating command and test files, always run:
 ```bash
-pnpm prepare        # Build + update manifest/README
+pnpm prepare        # Build + update manifest
+pnpm exec oclif readme  # Regenerate README.md from command metadata
 pnpm exec eslint .  # Lint (must be 0 errors)
 pnpm test:unit      # Run tests
 ```

.claude/skills/ably-new-command/references/testing.md

@@ -2,6 +2,8 @@
 
 Test files go at `test/unit/commands/<path-matching-command>.test.ts`.
 
+> **Note:** Do NOT pass `--duration` to `runCommand()` in unit/integration tests. `vitest.config.ts` sets `ABLY_CLI_DEFAULT_DURATION: "0.25"` which makes subscribe commands auto-exit after 250ms. Adding `--duration` overrides this with a slower value.
+
 ## Table of Contents
 - [Product API Test (Realtime Mock)](#product-api-test-realtime-mock)
 - [Product API Test (REST Mock)](#product-api-test-rest-mock)
@@ -62,7 +64,7 @@ describe("topic:action command", () => {
 
   describe("functionality", () => {
     it("should subscribe and display events", async () => {
-      const commandPromise = runCommand(["topic:action", "test-channel", "--duration", "1"], import.meta.url);
+      const commandPromise = runCommand(["topic:action", "test-channel"], import.meta.url);
 
       await vi.waitFor(() => { expect(mockCallback).not.toBeNull(); });
 
@@ -76,6 +78,28 @@ describe("topic:action command", () => {
       expect(stdout).toContain("test-channel");
     });
   });
+
+  describe("flags", () => {
+    it("should accept --duration flag", async () => {
+      const { stdout } = await runCommand(["topic:action", "--help"], import.meta.url);
+      expect(stdout).toContain("--duration");
+    });
+  });
+
+  describe("error handling", () => {
+    it("should handle connection failure", async () => {
+      const mock = getMockAblyRealtime();
+      mock.connection.once.mockImplementation((event: string, cb: (stateChange: unknown) => void) => {
+        if (event === "failed") cb({ reason: new Error("Connection failed") });
+      });
+
+      const { error } = await runCommand(
+        ["topic:action", "test-channel"],
+        import.meta.url,
+      );
+      expect(error).toBeDefined();
+    });
+  });
 });
 ```
 
@@ -99,13 +123,50 @@ describe("topic:action command", () => {
     });
   });
 
-  it("should retrieve history", async () => {
-    const { stdout } = await runCommand(
-      ["topic:action", "test-channel"],
-      import.meta.url,
-    );
-    expect(stdout).toContain("1");
-    expect(stdout).toContain("messages");
+  describe("help", () => {
+    it("should display help with --help flag", async () => {
+      const { stdout } = await runCommand(["topic:action", "--help"], import.meta.url);
+      expect(stdout).toContain("USAGE");
+    });
+  });
+
+  describe("argument validation", () => {
+    it("should require channel argument", async () => {
+      const { error } = await runCommand(["topic:action"], import.meta.url);
+      expect(error?.message).toMatch(/Missing .* required arg/i);
+    });
+  });
+
+  describe("functionality", () => {
+    it("should retrieve history", async () => {
+      const { stdout } = await runCommand(
+        ["topic:action", "test-channel"],
+        import.meta.url,
+      );
+      expect(stdout).toContain("1");
+      expect(stdout).toContain("messages");
+    });
+  });
+
+  describe("flags", () => {
+    it("should accept --json flag", async () => {
+      const { stdout } = await runCommand(["topic:action", "--help"], import.meta.url);
+      expect(stdout).toContain("--json");
+    });
+  });
+
+  describe("error handling", () => {
+    it("should handle API errors", async () => {
+      const mock = getMockAblyRest();
+      const channel = mock.channels._getChannel("test-channel");
+      channel.history.mockRejectedValue(new Error("API error"));
+
+      const { error } = await runCommand(
+        ["topic:action", "test-channel"],
+        import.meta.url,
+      );
+      expect(error).toBeDefined();
+    });
   });
 });
 ```
@@ -125,32 +186,61 @@ describe("topic:action command", () => {
     nock.cleanAll();
   });
 
-  it("should create resource", async () => {
-    const appId = getMockConfigManager().getCurrentAppId()!;
-    nock("https://control.ably.net")
-      .post(`/v1/apps/${appId}/resources`)
-      .reply(201, { id: "res-123", appId });
+  describe("help", () => {
+    it("should display help with --help flag", async () => {
+      const { stdout } = await runCommand(["topic:action", "--help"], import.meta.url);
+      expect(stdout).toContain("USAGE");
+    });
+  });
 
-    const { stdout } = await runCommand(
-      ["topic:action", "--flag", "value"],
-      import.meta.url,
-    );
+  describe("argument validation", () => {
+    it("should reject unknown flags", async () => {
+      const { error } = await runCommand(
+        ["topic:action", "--unknown-flag-xyz"],
+        import.meta.url,
+      );
+      expect(error).toBeDefined();
+      expect(error?.message).toMatch(/unknown|Nonexistent flag/i);
+    });
+  });
+
+  describe("functionality", () => {
+    it("should create resource", async () => {
+      const appId = getMockConfigManager().getCurrentAppId()!;
+      nock("https://control.ably.net")
+        .post(`/v1/apps/${appId}/resources`)
+        .reply(201, { id: "res-123", appId });
+
+      const { stdout } = await runCommand(
+        ["topic:action", "--flag", "value"],
+        import.meta.url,
+      );
+
+      expect(stdout).toContain("created");
+    });
+  });
 
-    expect(stdout).toContain("created");
+  describe("flags", () => {
+    it("should accept --json flag", async () => {
+      const { stdout } = await runCommand(["topic:action", "--help"], import.meta.url);
+      expect(stdout).toContain("--json");
+    });
   });
 
-  it("should handle API errors", async () => {
-    const appId = getMockConfigManager().getCurrentAppId()!;
-    nock("https://control.ably.net")
-      .post(`/v1/apps/${appId}/resources`)
-      .reply(400, { error: "Bad request" });
+  describe("error handling", () => {
+    it("should handle API errors", async () => {
+      const appId = getMockConfigManager().getCurrentAppId()!;
+      nock("https://control.ably.net")
+        .post(`/v1/apps/${appId}/resources`)
+        .reply(400, { error: "Bad request" });
 
-    const { error } = await runCommand(
-      ["topic:action", "--flag", "value"],
-      import.meta.url,
-    );
+      const { error } = await runCommand(
+        ["topic:action", "--flag", "value"],
+        import.meta.url,
+      );
 
-    expect(error).toBeDefined();
+      expect(error).toBeDefined();
+    });
   });
 });
 ```
@@ -266,9 +356,69 @@ describe.skipIf(SHOULD_SKIP_E2E)("topic:action CRUD E2E", { timeout: 30_000 }, (
 
 ## Test Structure
 
-Always include these describe blocks:
-1. `help` — verify `--help` shows USAGE, key flags, and EXAMPLES
-2. `argument validation` — verify required args produce errors when missing
-3. `functionality` — core behavior tests
-4. `flags` — verify key flags are accepted and configured
-5. `error handling` — API errors, invalid input
+Every test file MUST include all 5 of these describe blocks (exact names — no variants):
+
+1. **`help`** — verify `--help` shows USAGE, key flags, and EXAMPLES
+2. **`argument validation`** — verify required args produce errors when missing. For commands with no required args, test that unknown flags are rejected.
+3. **`functionality`** — core happy-path behavior tests. This is where the main command logic is tested.
+4. **`flags`** — verify key flags are accepted and configured (check help output contains flag names, test flag behavior)
+5. **`error handling`** — API errors, invalid input, network failures
+
+### Block naming rules
+- Use EXACTLY these names: `"help"`, `"argument validation"`, `"functionality"`, `"flags"`, `"error handling"`
+- Do NOT use variants like `"command arguments and flags"`, `"command flags"`, `"flag options"`, `"parameter validation"`, or domain-specific names like `"subscription behavior"`
+- Additional describe blocks beyond the 5 required ones are fine (e.g., `"JSON output"`, `"cleanup behavior"`)
+
+### What goes in each block
+
+**`argument validation`** for commands WITH required args:
+```typescript
+describe("argument validation", () => {
+  it("should require channel argument", async () => {
+    const { error } = await runCommand(["topic:action"], import.meta.url);
+    expect(error?.message).toMatch(/Missing .* required arg/i);
+  });
+});
+```
+
+**`argument validation`** for commands WITHOUT required args:
+```typescript
+describe("argument validation", () => {
+  it("should reject unknown flags", async () => {
+    const { error } = await runCommand(
+      ["topic:action", "--unknown-flag-xyz"],
+      import.meta.url,
+    );
+    expect(error).toBeDefined();
+    expect(error?.message).toMatch(/unknown|Nonexistent flag/i);
+  });
+});
+```
+
+**`argument validation`** for topic commands (that route to subcommands):
+```typescript
+describe("argument validation", () => {
+  it("should handle unknown subcommand gracefully", async () => {
+    const { stdout } = await runCommand(["topic", "nonexistent"], import.meta.url);
+    expect(stdout).toBeDefined();
+  });
+});
+```
+
+**`flags`** block pattern:
+```typescript
+describe("flags", () => {
+  it("should show available flags in help", async () => {
+    const { stdout } = await runCommand(["topic:action", "--help"], import.meta.url);
+    expect(stdout).toContain("--json");
+  });
+
+  it("should reject unknown flags", async () => {
+    const { error } = await runCommand(
+      ["topic:action", "test-arg", "--unknown-flag"],
+      import.meta.url,
+    );
+    expect(error).toBeDefined();
+  });
+});
+```

.claude/skills/ably-review/SKILL.md

@@ -121,9 +121,16 @@ For each changed command file, run the relevant checks. Spawn agents for paralle
 
 ### For changed test files (`test/unit/commands/**/*.ts`)
 
-1. **Grep** for `describe(` to check for required describe blocks: `help`, `argument validation`, `functionality`, `flags`, `error handling`
+1. **Grep** for `describe(` to check for the 5 required describe blocks with EXACT standard names:
+   - `describe("help"` — required in every test file
+   - `describe("argument validation"` — required (test required args OR unknown flag rejection)
+   - `describe("functionality"` — required (core happy-path tests)
+   - `describe("flags"` — required (verify flags exist and work)
+   - `describe("error handling"` — required (API errors, network failures)
+   Flag any non-standard variants: `"command arguments and flags"`, `"command flags"`, `"flag options"`, `"parameter validation"`. These must use the exact standard names above.
+   Exception: `interactive.test.ts`, `interactive-sigint.test.ts`, and `bench/*.test.ts` are exempt (REPL/benchmark tests, not command tests).
 2. **Grep** for `getMockAblyRealtime`, `getMockAblyRest`, `getMockConfigManager` to verify correct mock usage
-3. **Grep** for `--duration` in subscribe test files
+3. **Grep** for `--duration` in unit test `runCommand()` args — should NOT be present (env var handles it). Exceptions: `test:wait` tests, `interactive-sigint` tests, help output checks.
 4. **Grep** for `--api-key`, `--token`, `--access-token` — unit tests should not use CLI auth flags
 
 ### For new command files (added, not modified)

AGENTS.md

@@ -5,10 +5,12 @@
 **Run these IN ORDER for EVERY change:**
 
 ```bash
-pnpm prepare        # 1. Build + update manifest/README
-pnpm exec eslint .  # 2. Lint (MUST be 0 errors)
-pnpm test:unit      # 3. Test (at minimum)
-                    # 4. Update docs if needed
+pnpm prepare        # 1. Build + update manifest
+pnpm exec oclif readme  # 2. Regenerate README.md from command metadata
+pnpm exec eslint .  # 3. Lint (MUST be 0 errors)
+pnpm test:unit      # 4. Test (at minimum)
+pnpm test:tty       # 5. TTY tests (local only, skip in CI)
+                    # 6. Update docs if needed
 ```
 
 **If you skip these steps, the work is NOT complete.**
@@ -164,16 +166,35 @@ runCommand(["stats", "account"], {
 });
 ```
 
+**Duration in tests — do NOT use `--duration` in unit/integration tests:**
+Unit and integration tests set `ABLY_CLI_DEFAULT_DURATION: "0.25"` in `vitest.config.ts`, which makes all subscribe/long-running commands auto-exit after 250ms. Do NOT pass `--duration` to `runCommand()` — it overrides the fast 250ms default with a slower explicit value.
+
+Exceptions:
+- `test:wait` command tests — `--duration` is a required flag for that command
+- `interactive-sigint.test.ts` — needs a longer duration for SIGINT testing
+- Help output checks — testing that `--help` mentions `--duration` is fine
+
 **Test structure:**
 - `test/unit/` — Fast, mocked tests. Auth via `MockConfigManager` (automatic). Only set `ABLY_API_KEY` env var when testing env var override behavior.
 - `test/integration/` — Integration tests (e.g., interactive mode). Mocked external services but tests multi-component interaction.
+- `test/tty/` — Interactive mode tests requiring a real pseudo-TTY (e.g., SIGINT/Ctrl+C with readline). Uses `node-pty`. Local only — cannot run in CI.
 - `test/e2e/` — Full scenarios against real Ably. Auth via env vars (`ABLY_API_KEY`, `ABLY_ACCESS_TOKEN`).
 - Helpers in `test/helpers/` — `runCommand()`, `runLongRunningBackgroundProcess()`, `e2e-test-helper.ts`, `mock-config-manager.ts`.
 
+**Required test describe blocks** (exact names — every unit test file must have all 5):
+1. `"help"` — verify `--help` shows USAGE
+2. `"argument validation"` — test required args or unknown flag rejection
+3. `"functionality"` — core happy-path behavior
+4. `"flags"` — verify flags exist and work
+5. `"error handling"` — API errors, network failures
+
+Do NOT use variants like `"command arguments and flags"`, `"command flags"`, `"flag options"`, or `"parameter validation"`. Exempt: `interactive.test.ts`, `interactive-sigint.test.ts`, `bench/*.test.ts`.
+
 **Running tests:**
 ```bash
 pnpm test:unit                    # All unit tests
 pnpm test:integration             # Integration tests
+pnpm test:tty                     # TTY tests (local only, needs real terminal)
 pnpm test:e2e                     # All E2E tests
 pnpm test test/unit/commands/foo.test.ts  # Specific test
 ```

CONTRIBUTING.md

@@ -8,9 +8,9 @@ All code changes, whether features or bug fixes, **MUST** follow the mandatory w
 
 In summary, this involves:
 
-1.  **Build:** Run `pnpm prepare` to compile, update manifests, and update the README.
+1.  **Build:** Run `pnpm prepare` to compile and update manifests, then `pnpm exec oclif readme` to regenerate the README.
 2.  **Lint:** Run `pnpm exec eslint .` and fix all errors/warnings.
-3.  **Test:** Run relevant tests (`pnpm test:unit`, `pnpm test:integration`, `pnpm test:e2e`, `pnpm test:playwright`, or specific files) and ensure they pass. Add new tests or update existing ones as needed.
+3.  **Test:** Run relevant tests (`pnpm test:unit`, `pnpm test:integration`, `pnpm test:e2e`, `pnpm test:playwright`, or specific files) and ensure they pass. For interactive mode changes, also run `pnpm run test:tty` (requires a real terminal, not run in CI). Add new tests or update existing ones as needed.
 4.  **Document:** Update all relevant documentation (`docs/`, `AGENTS.md`, `README.md`) to reflect your changes.
 
 **Pull requests will not be merged unless all these steps are completed and verified.**