Skip to content
Merged
2 changes: 2 additions & 0 deletions .claude/skills/ably-codebase-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -154,13 +154,15 @@ Launch these agents **in parallel**. Each agent gets a focused mandate and uses
4. Cross-reference: every leaf command should appear in both the `logJsonResult`/`logJsonEvent` list and the `shouldOutputJson` list
5. **Read** streaming commands to verify they use `logJsonEvent`, one-shot commands use `logJsonResult`
6. **Read** each `logJsonResult`/`logJsonEvent` call and verify data is nested under a domain key — singular for events/single items (e.g., `{message: ...}`, `{cursor: ...}`), plural for collections (e.g., `{cursors: [...]}`, `{rules: [...]}`). Top-level envelope fields are `type`, `command`, `success` only. Metadata like `total`, `timestamp`, `appId` may sit alongside the domain key.
7. **Check** hold commands (set, enter, acquire) emit `logJsonStatus("holding", ...)` after `logJsonResult` — this signals to JSON consumers that the command is alive and waiting for Ctrl+C / `--duration`

**Reasoning guidance:**
- Commands that ONLY have human output (no JSON path) are deviations
- Direct `formatJsonRecord` usage in command files should use `logJsonResult`/`logJsonEvent` instead
- Topic index commands (showing help) don't need JSON output
- Data spread at the top level without a domain key is a deviation — nest under a singular or plural domain noun
- Metadata fields (`total`, `timestamp`, `hasMore`, `appId`) alongside the domain key are acceptable — they describe the result, not the domain objects
- Hold commands missing `logJsonStatus` after `logJsonResult` are deviations — JSON consumers need the hold signal

### Agent 6: Test Pattern Sweep

Expand Down
54 changes: 39 additions & 15 deletions .claude/skills/ably-new-command/references/patterns.md
Original file line number Diff line number Diff line change
Expand Up @@ -667,18 +667,15 @@ Commands must behave strictly according to their documented purpose — no unint
- **NOT enter presence/space** — `getAll()`, `get()` do NOT require `space.enter()`
- **NOT subscribe** to events or poll — fetch once, output, exit

**Set commands** — one-shot mutations:
- Enter space (required by SDK), set value, output, **exit**
- **NOT subscribe** after setting — that is what subscribe commands are for

**Enter / acquire commands** — hold state until Ctrl+C / `--duration`:
- Enter space, output confirmation with all relevant fields, then `waitAndTrackCleanup`
- **NOT subscribe** to other events
**Set / enter / acquire commands** — hold state until Ctrl+C / `--duration`:
- Enter space (manual: `enterSpace: false` + `space.enter()` + `markAsEntered()`), perform operation, output confirmation, then hold with `waitAndTrackCleanup`
- Emit `formatListening("Holding <thing>.")` (human) and `logJsonStatus("holding", ...)` (JSON)
- **NOT subscribe** to other events — that is what subscribe commands are for

**Side-effect rules:**
- `space.enter()` only when SDK requires it (set, enter, acquire)
- Call `this.markAsEntered()` after every `space.enter()` (enables cleanup)
- `initializeSpace(enterSpace: true)` calls `markAsEntered()` automatically
- For hold commands, always use manual entry (`enterSpace: false` + `space.enter()` + `markAsEntered()`) for consistency

```typescript
// WRONG — subscribe enters the space
Expand All @@ -696,14 +693,15 @@ const data = await this.space!.locations.getAll();
// CORRECT — get-all just fetches
const data = await this.space!.locations.getAll();

// WRONG — set command subscribes after setting
await this.space!.locations.set(location);
this.space!.locations.subscribe("update", handler); // NO
await this.waitAndTrackCleanup(flags, "location"); // NO

// CORRECT — set command exits after setting
// CORRECT — set command holds after setting
await this.initializeSpace(flags, spaceName, { enterSpace: false });
await this.space!.enter();
this.markAsEntered();
await this.space!.locations.set(location);
// run() completes, finally() handles cleanup
// output result...
this.log(formatListening("Holding location."));
this.logJsonStatus("holding", "Holding location. Press Ctrl+C to exit.", flags);
await this.waitAndTrackCleanup(flags, "location", flags.duration);
```

---
Expand Down Expand Up @@ -747,6 +745,32 @@ this.logJsonResult({ channels: items, total, hasMore }, flags); // channel

Metadata fields (`total`, `timestamp`, `hasMore`, `appId`) may sit alongside the collection key since they describe the result, not the domain objects.

### Hold status for long-running commands (logJsonStatus)

Long-running commands that hold state (e.g. `spaces members enter`, `spaces locations set`, `spaces locks acquire`, `spaces cursors set`) must emit a status line after the result so JSON consumers know the command is alive and waiting:

```typescript
// After the result output:
if (this.shouldOutputJson(flags)) {
this.logJsonResult({ member: formatMemberOutput(self!) }, flags);
} else {
this.log(formatSuccess(`Entered space: ${formatResource(spaceName)}.`));
// ... labels ...
this.log(formatListening("Holding presence."));
}

// logJsonStatus has built-in shouldOutputJson guard — no outer if needed
this.logJsonStatus("holding", "Holding presence. Press Ctrl+C to exit.", flags);

await this.waitAndTrackCleanup(flags, "member", flags.duration);
```

This emits two NDJSON lines in `--json` mode:
```jsonl
{"type":"result","command":"spaces:members:enter","success":true,"member":{...}}
{"type":"status","command":"spaces:members:enter","status":"holding","message":"Holding presence. Press Ctrl+C to exit."}
```

### Choosing the domain key name

| Scenario | Key | Example |
Expand Down
1 change: 1 addition & 0 deletions .claude/skills/ably-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,7 @@ For each changed command file, run the relevant checks. Spawn agents for paralle
3. **Grep** for `shouldOutputJson` — verify human output is guarded
4. **Read** the file to verify streaming commands use `logJsonEvent` and one-shot commands use `logJsonResult`
5. **Read** `logJsonResult`/`logJsonEvent` call sites and check data is nested under a domain key (singular for events/single items, plural for collections) — not spread at top level. Top-level envelope fields are `type`, `command`, `success` only. Metadata like `total`, `timestamp`, `appId` may sit alongside the domain key.
6. **Check** hold commands (set, enter, acquire) emit `logJsonStatus("holding", ...)` after `logJsonResult` — this signals to JSON consumers that the command is alive and waiting for Ctrl+C / `--duration`

**Control API helper check (grep — for Control API commands only):**
1. **Grep** for `resolveAppId` — should use `requireAppId` instead (encapsulates null check and `fail()`)
Expand Down
7 changes: 4 additions & 3 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,8 @@ All output helpers use the `format` prefix and are exported from `src/utils/outp
- **Pagination warning**: `formatPaginationWarning(pagesConsumed, itemCount, isBillable?)` — shows "Fetched N pages" when `pagesConsumed > 1`. Pass `isBillable: true` for history commands (billable API calls). Guard with `!this.shouldOutputJson(flags)`.
- **Pagination next hint**: `buildPaginationNext(hasMore, lastTimestamp?)` — returns `{ hint, start? }` for JSON output when `hasMore` is true. Pass `lastTimestamp` only for history commands (which have `--start`).
- **JSON guard**: All human-readable output (progress, success, listening messages) must be wrapped in `if (!this.shouldOutputJson(flags))` so it doesn't pollute `--json` output. Only JSON payloads should be emitted when `--json` is active.
- **JSON envelope**: Use `this.logJsonResult(data, flags)` for one-shot results and `this.logJsonEvent(data, flags)` for streaming events. The envelope adds three top-level fields (`type`, `command`, `success?`). Nest domain data under a **domain key** (see "JSON data nesting convention" below). Do NOT add ad-hoc `success: true/false` — the envelope handles it. `--json` produces compact single-line output (NDJSON for streaming). `--pretty-json` is unchanged.
- **JSON envelope**: Use `this.logJsonResult(data, flags)` for one-shot results, `this.logJsonEvent(data, flags)` for streaming events, and `this.logJsonStatus(status, message, flags)` for hold/status signals in long-running commands. The envelope adds top-level fields (`type`, `command`, `success?`). Nest domain data under a **domain key** (see "JSON data nesting convention" below). Do NOT add ad-hoc `success: true/false` — the envelope handles it. `--json` produces compact single-line output (NDJSON for streaming). `--pretty-json` is unchanged.
- **JSON hold status**: Long-running hold commands (e.g. `spaces members enter`, `spaces locations set`, `spaces locks acquire`, `spaces cursors set`) must emit a `logJsonStatus("holding", "Holding <thing>. Press Ctrl+C to exit.", flags)` line after the result. This tells LLM agents and scripts that the command is alive and waiting. `logJsonStatus` has a built-in `shouldOutputJson` guard — no outer `if` needed.
- **JSON errors**: Use `this.fail(error, flags, component, context?)` as the single error funnel in command `run()` methods. It logs the CLI event, preserves structured error data (Ably codes, HTTP status), emits JSON error envelope when `--json` is active, and calls `this.error()` for human-readable output. Returns `never` — no `return;` needed after calling it. Do NOT call `this.error()` directly — it is an internal implementation detail of `fail`.
- **History output**: Use `[index] [timestamp]` on the same line as a heading: `` `${formatIndex(index + 1)} ${formatTimestamp(timestamp)}` ``, then fields indented below. This is distinct from **get-all output** which uses `[index]` alone on its own line. See `references/patterns.md` "History results" and "One-shot results" for both patterns.

Expand All @@ -246,9 +247,9 @@ See `references/patterns.md` "JSON Data Nesting Convention" in the `ably-new-com
Each command type has strict rules about what side effects it may have. Remove unintended side effects (e.g., auto-entering presence) and support passive ("dumb") operations where applicable. Key principles:
- **Subscribe** = passive observer (no `space.enter()`, no fetching initial state)
- **Get-all / get** = one-shot query (no `space.enter()`, no subscribing)
- **Set** = one-shot mutation (enter, set, exit — no subscribing after)
- **Enter / acquire** = hold state until Ctrl+C / `--duration`
- **Set / enter / acquire** = hold state until Ctrl+C / `--duration` (enter, operate, hold — no subscribing after)
- Call `space.enter()` only when SDK requires it; always call `this.markAsEntered()` after
- Hold commands use manual entry (`enterSpace: false` + `space.enter()` + `markAsEntered()`) for consistency

See `references/patterns.md` "Command behavior semantics" in the `ably-new-command` skill for full rules, side-effect table, and code examples.

Expand Down
Loading
Loading