Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion .claude/skills/ably-codebase-review/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,23 @@ Launch these agents **in parallel**. Each agent gets a focused mandate and uses
5. **Read** command files and look for unguarded `this.log()` calls (not inside `if (!this.shouldOutputJson(flags))`)
6. **Grep** for quoted resource names patterns like `"${` or `'${` near `channel`, `name`, `app` variables — should use `formatResource()`
**Method (grep — structured output format):**
7. **Grep** for box-drawing characters (`┌`, `┬`, `├`, `└`, `│`) in command files — non-JSON output must use multi-line labeled blocks, not ASCII tables or grids
8. **Grep** for subscribe commands that call `getAll()` or equivalent before subscribing — subscribe commands must NOT fetch initial state (they should only listen for new events)
9. For data-outputting commands, **read** both the JSON and non-JSON output paths and compare fields — non-JSON should expose the same fields as JSON mode (omitting only null/empty values)
10. **Grep** for local `interface` definitions in `src/commands/` that duplicate SDK types (e.g., `interface CursorPosition`, `interface CursorData`, `interface PresenceMessage`) — these should import from `ably`, `@ably/spaces`, or `@ably/chat` instead. Display/output interfaces in `src/utils/` are intentional and fine.
**Method (LSP — for completeness mapping):**
7. Use `LSP findReferences` on `shouldOutputJson` to get the definitive list of all commands that check for JSON mode — cross-reference against the full command list to find commands missing JSON guards
11. Use `LSP findReferences` on `shouldOutputJson` to get the definitive list of all commands that check for JSON mode — cross-reference against the full command list to find commands missing JSON guards
**Reasoning guidance:**
- List commands don't use `formatSuccess()` (no action to confirm) — this is correct, not a deviation
- `chalk.red("✗")` / `chalk.green("✓")` as visual indicators in test/bench output is acceptable
- `chalk.yellow("Warning: ...")` should use `formatWarning()` instead — `formatWarning` adds the `⚠` symbol automatically and "Warning:" prefix is unnecessary
- ASCII tables/grids in non-JSON output are deviations — use multi-line labeled blocks with `formatLabel()` instead
- Subscribe commands fetching initial state (via `getAll()`, `getSelf()`, etc.) before subscribing are deviations — subscribe should only listen for new events
- Non-JSON output that hides fields available in JSON mode is a deviation — both modes should expose the same data
- Local `interface` definitions in command files that duplicate SDK types are deviations — import from the SDK package instead. Display/output interfaces in `src/utils/` (e.g., `MemberOutput`, `MessageDisplayFields`) are intentional transformations, not duplicates.
### Agent 4: Flag Architecture Sweep
Expand Down Expand Up @@ -143,11 +153,14 @@ Launch these agents **in parallel**. Each agent gets a focused mandate and uses
3. **Grep** for `shouldOutputJson` in command files to find all JSON-aware commands
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.
**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
### Agent 6: Test Pattern Sweep
Expand Down
23 changes: 19 additions & 4 deletions .claude/skills/ably-new-command/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -176,14 +176,23 @@ if (!this.shouldOutputJson(flags)) {
this.log(formatListening("Listening for messages."));
}

// JSON output — use logJsonResult for one-shot results:
// JSON output — nest data under a domain key, not spread at top level.
// Envelope provides top-level: type, command, success.
// One-shot single result — singular domain key:
if (this.shouldOutputJson(flags)) {
this.logJsonResult({ channel: args.channel, message }, flags);
this.logJsonResult({ message: messageData }, flags);
// → {"type":"result","command":"...","success":true,"message":{...}}
}

// Streaming events — use logJsonEvent:
// One-shot collection result — plural domain key + optional metadata:
if (this.shouldOutputJson(flags)) {
this.logJsonEvent({ eventType: event.type, message, channel: channelName }, flags);
this.logJsonResult({ messages: items, total: items.length }, flags);
}

// Streaming events — singular domain key:
if (this.shouldOutputJson(flags)) {
this.logJsonEvent({ message: messageData }, flags);
// → {"type":"event","command":"...","message":{...}}
}
```

Expand Down Expand Up @@ -389,8 +398,14 @@ See the "Keeping Skills Up to Date" section in `CLAUDE.md` for the full list of
- [ ] All human output wrapped in `if (!this.shouldOutputJson(flags))`
- [ ] Output helpers used correctly (`formatProgress`, `formatSuccess`, `formatWarning`, `formatListening`, `formatResource`, `formatTimestamp`, `formatClientId`, `formatEventType`, `formatLabel`, `formatHeading`, `formatIndex`)
- [ ] `formatSuccess()` messages end with `.` (period)
- [ ] Non-JSON data output uses multi-line labeled blocks (see `patterns.md` "Human-Readable Output Format"), not tables or custom grids
- [ ] Non-JSON output exposes all available SDK fields (same data as JSON mode, omitting only null/empty values)
- [ ] SDK types imported directly (`import type { CursorUpdate } from "@ably/spaces"`) — no local interface redefinitions of SDK types (display interfaces in `src/utils/` are fine)
- [ ] Field coverage checked against SDK type definitions (`node_modules/ably/ably.d.ts`, `node_modules/@ably/spaces/dist/mjs/types.d.ts`)
- [ ] Subscribe commands do NOT fetch initial state — they only listen for new events (use `get-all` for current state)
- [ ] Resource names use `formatResource(name)`, never quoted
- [ ] JSON output uses `logJsonResult()` (one-shot) or `logJsonEvent()` (streaming), not direct `formatJsonRecord()`
- [ ] JSON data nested under a domain key (singular for events/single items, plural for collections) — not spread at top level (see `patterns.md` "JSON Data Nesting Convention")
- [ ] Subscribe/enter commands use `this.waitAndTrackCleanup(flags, component, flags.duration)` (not `waitUntilInterruptedOrTimeout`)
- [ ] Error handling uses `this.fail()` exclusively, not `this.error()` or `this.log(chalk.red(...))`
- [ ] Component strings are camelCase: single-word lowercase (`"room"`, `"auth"`), multi-word camelCase (`"channelPublish"`, `"roomPresenceSubscribe"`)
Expand Down
Loading