Skip to content

[DX-264] Rename channel-rules to apps rules and alias legacy command paths#164

Merged
umair-ably merged 4 commits intomainfrom
DX-264-rules
Mar 12, 2026
Merged

[DX-264] Rename channel-rules to apps rules and alias legacy command paths#164
umair-ably merged 4 commits intomainfrom
DX-264-rules

Conversation

@umair-ably
Copy link
Copy Markdown
Collaborator

@umair-ably umair-ably commented Mar 12, 2026

Summary

  • Rename apps channel-rules to apps rules as the canonical command path for managing channel rules (namespaces)
  • Convert apps channel-rules and channel-rule commands into hidden alias wrappers that delegate to apps rules
  • Add formatResource() exception guidance to the ably-new-command skill (don't use ANSI helpers inside this.fail())

Review strategy

Step What to review Why
1 src/commands/apps/rules/*.ts Canonical commands — all real logic lives here. Compare against old apps/channel-rules/ implementations to confirm nothing was lost.
2 One alias wrapper (e.g. src/commands/channel-rule/create.ts) All 10 wrappers follow the same 17-line delegation pattern. Verify one, spot-check the rest.
3 test/unit/commands/apps/rules/*.test.ts Full test coverage (5 required describe blocks, standard helpers, nock mocks).
4 Alias test files (apps/channel-rules/*.test.ts, channel-rule/*.test.ts) Only verify forwarding — intentionally minimal since testing aliases end-to-end would duplicate canonical tests.
5 src/utils/channel-rule-display.ts Shared display helper — check for changes beyond import path updates.
6 docs/Project-Structure.md, .claude/skills/ably-new-command/SKILL.md Docs reflect new paths. Skill adds one-line exception about formatResource() inside this.fail().

Test plan

  • pnpm prepare succeeds
  • pnpm exec eslint . shows 0 errors
  • pnpm test:unit passes — specifically test/unit/commands/apps/rules/ and alias forwarding tests
  • ably apps rules list works end-to-end
  • ably apps channel-rules list still works (alias)
  • ably channel-rule list still works (alias)
  • ably apps rules --help shows correct subcommands
  • Hidden aliases don't appear in ably --help or ably apps --help

Summary by CodeRabbit

  • New Features

    • Introduced unified "ably apps rules" commands: rules create, list, update, delete.
  • Documentation

    • Updated docs and README examples to use "ably apps rules" and clarified project structure.
    • Guidance added: error messages must use plain strings (no ANSI) to avoid corrupting JSON envelopes.
  • Deprecation

    • Former "ably apps channel-rules" commands now act as hidden aliases to the new rules commands.
  • Bug Fixes

    • Improved error handling to avoid duplicate processing and corrupted JSON error output.

@vercel
Copy link
Copy Markdown

vercel bot commented Mar 12, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
cli-web-cli Ready Ready Preview, Comment Mar 12, 2026 3:25pm

Request Review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 12, 2026

Important

Review skipped

Auto incremental reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: d0f96e56-ec36-45ba-a2fc-d921d2b56906

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Walkthrough

Refactors channel-rules into a new apps:rules namespace: full-featured rules commands added; former apps:channel-rules and channel-rule commands converted to hidden alias wrappers that delegate to the new implementations. Tests updated: comprehensive suites for rules, simplified alias tests; docs/examples updated.

Changes

Cohort / File(s) Summary
New Rules Command Implementations
src/commands/apps/rules/index.ts, src/commands/apps/rules/create.ts, src/commands/apps/rules/list.ts, src/commands/apps/rules/update.ts, src/commands/apps/rules/delete.ts
Added full implementations for apps:rules topic and subcommands (create/list/update/delete) with flags, API integration, JSON/human-readable output, validation, and error handling.
Apps:channel-rules → Alias Wrappers
src/commands/apps/channel-rules/*.ts (create.ts, delete.ts, index.ts, list.ts, update.ts)
Replaced prior ControlBaseCommand implementations with thin alias wrappers that extend Command, mark hidden/isAlias, and delegate execution to corresponding apps:rules commands.
Legacy channel-rule aliases
src/commands/channel-rule/*.ts (create.ts, delete.ts, index.ts, list.ts, update.ts)
Updated legacy channel-rule commands to forward to apps:rules implementations (imports, static metadata, and run delegation updated).
Tests — New rules suites
test/unit/commands/apps/rules/*.test.ts (create.test.ts, list.test.ts, update.test.ts, delete.test.ts, index.test.ts)
Added comprehensive unit tests for the new apps:rules commands covering functionality, JSON output, validations, and control-API error paths.
Tests — Alias simplifications
test/unit/commands/apps/channel-rules/*.test.ts, test/unit/commands/channel-rule/*.test.ts
Simplified alias-focused tests: reduced coverage to forwarding behavior; removed detailed scenarios now covered by apps:rules tests.
Base command error handling
src/base-command.ts
Added an early rethrow guard in AblyBaseCommand.fail to rethrow oclif-marked errors and avoid double-processing when fail() is invoked inside try blocks.
Test fixtures and utilities
test/fixtures/control-api.ts, src/utils/channel-rule-display.ts
Extended mock namespace fixture with optional mutableMessages; refactored display labels to use shared formatLabel helper.
Documentation & README updates
README.md, docs/Project-Structure.md, .claude/skills/*, src/commands/apps/index.ts
Updated README/examples and project-structure docs to reference apps:rules (with channel-rules as hidden alias); added review guidance SKILL.md entries; updated example in apps index.

Sequence Diagram(s)

sequenceDiagram
  participant CLI as "User CLI"
  participant Alias as "Alias Command\n(e.g. apps:channel-rules)"
  participant Rules as "apps:rules Command"
  participant Control as "Control API"

  CLI->>Alias: invoke alias (e.g. apps:channel-rules create ...)
  Alias->>Rules: delegate (new Rules(...).run())
  Rules->>Control: POST/GET/PATCH/DELETE /v1/apps/{appId}/namespaces
  Control-->>Rules: HTTP response (200/201/204 or error)
  Rules-->>CLI: formatted output (JSON or human-readable) or this.fail(error)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Poem

🐰 Hops and patches, tidy and neat,

Rules born anew, aliases keep the seat,
Tests multiplied where logic grew,
Old paths forward, the CLI says "phew",
A carrot-toast to refactors true!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically describes the main change: renaming channel-rules to apps rules and establishing alias wrappers for legacy command paths.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch DX-264-rules
📝 Coding Plan for PR comments
  • Generate coding plan

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR renames the primary “channel rules” command surface area from apps channel-rules to apps rules, while keeping legacy command paths working via hidden aliases. It also aligns channel rule detail output formatting with the shared formatLabel() helper and updates docs/tests accordingly.

Changes:

  • Introduce new primary commands under src/commands/apps/rules/* (create/list/update/delete + topic).
  • Convert legacy commands (apps/channel-rules/* and channel-rule/*) into hidden alias commands that forward to apps:rules:*.
  • Update channel rule detail formatting and regenerate/update documentation and unit tests.

Reviewed changes

Copilot reviewed 33 out of 33 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/commands/apps/rules/create.ts Adds the new apps:rules:create command implementation.
src/commands/apps/rules/list.ts Adds the new apps:rules:list command implementation (including JSON output).
src/commands/apps/rules/update.ts Adds the new apps:rules:update command implementation with validation + JSON/human output.
src/commands/apps/rules/delete.ts Adds the new apps:rules:delete command implementation with confirmation flow + JSON/human output.
src/commands/apps/rules/index.ts Adds the apps:rules topic command.
src/commands/apps/channel-rules/{index,create,list,update,delete}.ts Converts legacy apps:channel-rules:* commands into hidden forwarders to apps:rules:*.
src/commands/channel-rule/{index,create,list,update,delete}.ts Converts legacy channel-rule:* commands into hidden forwarders to apps:rules:*.
src/utils/channel-rule-display.ts Switches detail line formatting to use formatLabel() consistently.
test/unit/commands/apps/rules/*.test.ts Adds new unit tests for the apps:rules:* commands.
test/unit/commands/apps/channel-rules/*.test.ts Updates legacy-path tests to validate forwarding behavior.
test/unit/commands/channel-rule/*.test.ts Updates legacy-path tests to validate forwarding behavior.
docs/Project-Structure.md Documents apps/rules as the primary path and legacy paths as hidden aliases.
README.md Updates generated CLI docs to reflect apps rules as the primary command group.
.claude/skills/ably-new-command/SKILL.md Clarifies ANSI/formatResource() usage constraints in fail() messages.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

@umair-ably
Copy link
Copy Markdown
Collaborator Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 12, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 13

🧹 Nitpick comments (1)
test/unit/commands/apps/rules/list.test.ts (1)

71-85: Test doesn't verify mutableMessages display.

The test description says "should display mutableMessages in rule details" but the mock doesn't set mutableMessages: true and the assertion only checks for the rule id, not the "Mutable Messages" label. Based on src/utils/channel-rule-display.ts, the field is only displayed when rule.mutableMessages !== undefined.

💡 Suggested fix to actually test mutableMessages display
     it("should display mutableMessages in rule details", async () => {
       const appId = getMockConfigManager().getCurrentAppId()!;
       nockControl()
         .get(`/v1/apps/${appId}/namespaces`)
         .reply(200, [
           mockNamespace({
             id: "mutable-chat",
             persisted: true,
+            mutableMessages: true,
           }),
         ]);

       const { stdout } = await runCommand(["apps:rules:list"], import.meta.url);

       expect(stdout).toContain("mutable-chat");
+      expect(stdout).toContain("Mutable Messages:");
     });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/unit/commands/apps/rules/list.test.ts` around lines 71 - 85, The test
"should display mutableMessages in rule details" currently only creates a mock
namespace via mockNamespace({ id: "mutable-chat", persisted: true }) and asserts
the id appears; update the mock to include mutableMessages: true (e.g.,
mockNamespace({... mutableMessages: true })) so the code path that renders the
"Mutable Messages" label (checked via rule.mutableMessages in
src/utils/channel-rule-display.ts) is exercised, then change the assertion to
check stdout contains the visible label/text for mutable messages (e.g.,
"Mutable Messages" or whatever string the display code emits) after running
runCommand(["apps:rules:list"], import.meta.url).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.claude/skills/ably-new-command/SKILL.md:
- Line 214: Update the checklist entry about quoting resource names to include
the new exception for this.fail(): keep the rule that resource names are
normally unquoted and use formatResource(name) for display, but explicitly add
that inside this.fail() messages you must not use formatResource() or any
ANSI-producing helper and must use plain quoted strings instead; reference
formatResource(name) and this.fail() in the checklist text so future scaffolds
follow the exception.

In `@README.md`:
- Around line 420-442: The fenced code blocks showing CLI help (blocks starting
with "USAGE", "FLAGS", "DESCRIPTION", "EXAMPLES") are missing a language tag and
trigger markdownlint MD040; update each triple-backtick fence around those help
snippets to use a language tag (use "text") so they become ```text ... ```;
ensure you apply this change to all similar help blocks (the blocks around the
usages/examples referenced in the comment) and, if these blocks are generated,
make the change in the generator/template that emits them rather than only
editing the README.md instance.

In `@src/commands/apps/rules/delete.ts`:
- Around line 49-62: The not-found error for channel rules is invoking
this.fail() inside the try block (after calling this.createControlApi and
controlApi.listNamespaces) and then being handled again in the catch, causing
duplicate error output; fix it by moving the namespace-not-found guard out of
the try/catch (or avoid calling this.fail() inside the try and instead throw a
plain Error so the outer catch can call this.fail() once). Locate the logic
using createControlApi, controlApi.listNamespaces(appId) and the namespace check
(namespace === undefined) in the delete command and either perform the namespace
existence check before entering the try/catch or replace the internal
this.fail() with a thrown Error so only the outer catch calls this.fail().

In `@src/commands/apps/rules/update.ts`:
- Around line 117-149: The try block around control API calls and subsequent
validations is catching errors thrown by this.fail() (used in the not-found,
invalid persistence, and no-op update guards), causing duplicate JSON/fatal
logging; to fix, perform the namespace lookup and all preflight validations (the
namespace existence check using listNamespaces/find with args.nameOrId, the
persistence vs mutable-messages validation that reads namespace.mutableMessages,
and the no-op update check) outside the try that wraps network calls (or rethrow
Command/Exit errors immediately), so keep createControlApi and listNamespaces
inside a minimal try but move the this.fail() guards (and any logic referencing
namespace or flags like flags.persisted and flags["mutable-messages"]) before
the catchable block to avoid swallowing and re-emitting the same failure.

In `@src/utils/channel-rule-display.ts`:
- Line 4: The import of formatLabel() changed visible labels by adding a
trailing ":" and dimming; restore the previous label text in this file by either
using the original literal labels (e.g., "Persisted", "Created", etc.) instead
of formatLabel(), or by calling formatLabel() then removing the appended
colon/dimming so the output matches the prior shape; update all call sites in
this module (the functions that render/display channel rule details such as the
label calls around "Persisted"/"Created" and the other label lines between lines
42–124) and adjust any tests/expectations that depended on the old output.

In `@test/unit/commands/apps/channel-rules/create.test.ts`:
- Around line 16-24: Prettier flagged formatting in the nock setup: reformat the
nockControl() chain and the .reply(201, { ... }) object so it matches project
Prettier rules; specifically locate the nockControl() call with
.post(`/v1/apps/${appId}/namespaces`) and the subsequent .reply(201, { id:
"chat", persisted: false, pushEnabled: false, created: Date.now(), modified:
Date.now() }) and run prettier --write on the file (or adjust spacing/line
breaks to match Prettier) to resolve the CI formatting error.

In `@test/unit/commands/apps/channel-rules/delete.test.ts`:
- Around line 9-14: The test file currently uses a single
describe("apps:channel-rules:delete alias") block; split it into the required
five describe blocks named exactly "help", "argument validation",
"functionality", "flags", and "error handling", then move the existing
forwarding assertion (the it(...) that checks forwarding to apps:rules:delete)
into describe("functionality", ...) while leaving the other four describe blocks
present (they can be empty or contain appropriate placeholder tests/afterEach
controlApiCleanup as needed); ensure you keep the existing afterEach cleanup and
the test's assertions intact and that the describe names match the specification
exactly.
- Around line 28-30: The nock chain is split across lines causing formatter
failures; collapse the chain so the call is on one line by combining
nockControl(), .delete(`/v1/apps/${appId}/namespaces/chat`), and .reply(204)
into a single statement (e.g., nockControl().delete(...).reply(204)) so that the
formatter accepts the line; update the usage of nockControl(), .delete(...) and
.reply(204) accordingly.

In `@test/unit/commands/apps/channel-rules/list.test.ts`:
- Around line 9-14: The test file currently uses a single
describe("apps:channel-rules:list alias") block but must be refactored to
contain exactly five top-level describe blocks with the exact names "help",
"argument validation", "functionality", "flags", and "error handling"; move the
existing afterEach and the it("should forward to apps:rules:list...") test into
a new describe("functionality", ...) block (preserving controlApiCleanup and the
test body), and add empty or placeholder describe blocks for "help", "argument
validation", "flags", and "error handling" so the file conforms to the required
five-block structure without changing the test logic in the moved it block.

In `@test/unit/commands/apps/channel-rules/update.test.ts`:
- Around line 28-31: The nock mock chain starting with nockControl() and the
chained .patch(`/v1/apps/${appId}/namespaces/chat`) call has inconsistent
formatting and reply body indentation; reformat the chain to match the rest of
the test suite (align chained methods on the same indentation level and indent
the reply body object consistently), e.g. ensure nockControl() is followed by a
properly indented .patch(...) line and the .reply(200, { id: "chat", ... })
object keys are indented to match other mocks in the file, updating the block
around the .patch and .reply calls to conform to the project's formatter rules.
- Around line 9-14: The test file currently only has
describe("apps:channel-rules:update alias"); restore the required five top-level
describe blocks named exactly "help", "argument validation", "functionality",
"flags", and "error handling", and move the existing forwarding test into
describe("functionality", ...). Ensure each describe block exists (even if some
are empty) and keep the forwarding assertion inside the test that lives in the
"functionality" describe; verify no additional top-level describe names are
present and follow the existing test patterns used elsewhere in the repo.

In `@test/unit/commands/apps/rules/create.test.ts`:
- Around line 16-33: The top-level describe must use the repo-standard suite
names instead of a custom title; remove or replace describe("apps:rules:create
command") and reorganize the test file into exactly five describe blocks named
'help', 'argument validation', 'functionality', 'flags', and 'error handling'.
Move the existing calls like standardHelpTests("apps:rules:create",
import.meta.url), standardArgValidationTests(...), standardFlagTests(...) and
any controlApiCleanup/afterEach into their appropriate describe blocks (e.g.,
put standardHelpTests in the 'help' block, standardArgValidationTests in
'argument validation', standardFlagTests in 'flags', and actual behavior tests
under 'functionality' with errors under 'error handling'), ensuring the file
contains exactly those five describe blocks and no other top-level describe
titles.
- Around line 125-147: The test never verifies that mutableMessages is
serialized because the mocked response (mockNamespace) does not include
mutableMessages and the assertions only check persisted; update the nock/post
body check and mocked reply to include mutableMessages and add an assertion on
the parsed JSON result to verify rule.mutableMessages is true: adjust the
nockControl().post callback to expect body.mutableMessages === true, have
mockNamespace(...) include mutableMessages: true in its returned object, and add
expect(result.rule).toHaveProperty("mutableMessages", true) after parsing stdout
from runCommand.

---

Nitpick comments:
In `@test/unit/commands/apps/rules/list.test.ts`:
- Around line 71-85: The test "should display mutableMessages in rule details"
currently only creates a mock namespace via mockNamespace({ id: "mutable-chat",
persisted: true }) and asserts the id appears; update the mock to include
mutableMessages: true (e.g., mockNamespace({... mutableMessages: true })) so the
code path that renders the "Mutable Messages" label (checked via
rule.mutableMessages in src/utils/channel-rule-display.ts) is exercised, then
change the assertion to check stdout contains the visible label/text for mutable
messages (e.g., "Mutable Messages" or whatever string the display code emits)
after running runCommand(["apps:rules:list"], import.meta.url).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: e7318ee3-60f1-44a5-a06b-4b3f385a36fb

📥 Commits

Reviewing files that changed from the base of the PR and between 5eca645 and f8ecbdc.

📒 Files selected for processing (33)
  • .claude/skills/ably-new-command/SKILL.md
  • README.md
  • docs/Project-Structure.md
  • src/commands/apps/channel-rules/create.ts
  • src/commands/apps/channel-rules/delete.ts
  • src/commands/apps/channel-rules/index.ts
  • src/commands/apps/channel-rules/list.ts
  • src/commands/apps/channel-rules/update.ts
  • src/commands/apps/index.ts
  • src/commands/apps/rules/create.ts
  • src/commands/apps/rules/delete.ts
  • src/commands/apps/rules/index.ts
  • src/commands/apps/rules/list.ts
  • src/commands/apps/rules/update.ts
  • src/commands/channel-rule/create.ts
  • src/commands/channel-rule/delete.ts
  • src/commands/channel-rule/index.ts
  • src/commands/channel-rule/list.ts
  • src/commands/channel-rule/update.ts
  • src/utils/channel-rule-display.ts
  • test/unit/commands/apps/channel-rules/create.test.ts
  • test/unit/commands/apps/channel-rules/delete.test.ts
  • test/unit/commands/apps/channel-rules/list.test.ts
  • test/unit/commands/apps/channel-rules/update.test.ts
  • test/unit/commands/apps/rules/create.test.ts
  • test/unit/commands/apps/rules/delete.test.ts
  • test/unit/commands/apps/rules/index.test.ts
  • test/unit/commands/apps/rules/list.test.ts
  • test/unit/commands/apps/rules/update.test.ts
  • test/unit/commands/channel-rule/create.test.ts
  • test/unit/commands/channel-rule/delete.test.ts
  • test/unit/commands/channel-rule/list.test.ts
  • test/unit/commands/channel-rule/update.test.ts

@sacOO7
Copy link
Copy Markdown
Contributor

sacOO7 commented Mar 12, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Mar 12, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
test/unit/commands/apps/rules/list.test.ts (2)

71-85: Test name doesn't match assertion coverage.

The test is titled "should display mutableMessages in rule details" but only asserts that stdout contains the rule name "mutable-chat". It doesn't verify that mutableMessages is actually displayed in the output. Either rename the test to reflect what it actually verifies, or add an assertion for the mutableMessages display (e.g., expect(stdout).toContain("Mutable Messages")).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/unit/commands/apps/rules/list.test.ts` around lines 71 - 85, The test
"should display mutableMessages in rule details" only asserts that stdout
contains the namespace name "mutable-chat" (via mockNamespace and
runCommand(["apps:rules:list"])), so either rename the test to reflect that it
verifies namespace display or add an assertion that checks for the
mutableMessages output (for example add expect(stdout).toContain("Mutable
Messages") or the exact label/string your CLI prints for mutableMessages) so the
title matches coverage; update the test name or add the extra expect referencing
runCommand, mockNamespace, and stdout accordingly.

104-121: Strengthen JSON output assertion for mutableMessages.

The test "should include mutableMessages in JSON output" verifies rules array length but doesn't assert that mutableMessages is present or has the expected value. Consider adding explicit assertions:

🔧 Proposed fix
       const result = JSON.parse(stdout);
       expect(result).toHaveProperty("success", true);
       expect(result.rules).toHaveLength(2);
+      expect(result.rules[0]).toHaveProperty("mutableMessages");
+      expect(result.rules[1]).toHaveProperty("mutableMessages");
     });
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@test/unit/commands/apps/rules/list.test.ts` around lines 104 - 121, Update
the "should include mutableMessages in JSON output" test to explicitly assert
that the JSON result includes the mutableMessages field and its expected value
for the mutable-chat namespace: after parsing stdout into result, locate the
rule object for the namespace created via mockNamespace({ id: "mutable-chat",
persisted: true }) and add expects that rule.mutableMessages is present and true
(and optionally assert mutableMessages is false or absent for the regular-chat
rule); reference the test name "should include mutableMessages in JSON output",
the runCommand call, and the mockNamespace usages to find where to add these
assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In @.claude/skills/ably-review/SKILL.md:
- Line 92: Scope the lint rule to only flag catch blocks that use the manual
oclif re-throw guard when the command actually inherits the shared fail()
implementation: perform an LSP/AST resolution to confirm that the catch's
this.fail resolves to the base class method (e.g., BaseCommand.fail) before
reporting; only then treat patterns like `if (error instanceof Error && "oclif"
in error) throw error` as redundant and suggest removal. Ensure the resolver
checks class inheritance/override status so commands that override fail() are
excluded from the rule.

In `@src/commands/apps/channel-rules/list.ts`:
- Around line 12-14: The current run() in this command directly constructs
RulesList and calls run(), bypassing oclif lifecycle hooks; change it to
delegate to oclif via this.config.runCommand(...) instead of new RulesList(...)
so init()/finally() on the shared base command run correctly—invoke
this.config.runCommand with the canonical command identifier for "ably apps
rules list" (or the RulesList command class) and pass the existing argv/flags so
the dispatcher handles lifecycle and execution.

---

Nitpick comments:
In `@test/unit/commands/apps/rules/list.test.ts`:
- Around line 71-85: The test "should display mutableMessages in rule details"
only asserts that stdout contains the namespace name "mutable-chat" (via
mockNamespace and runCommand(["apps:rules:list"])), so either rename the test to
reflect that it verifies namespace display or add an assertion that checks for
the mutableMessages output (for example add expect(stdout).toContain("Mutable
Messages") or the exact label/string your CLI prints for mutableMessages) so the
title matches coverage; update the test name or add the extra expect referencing
runCommand, mockNamespace, and stdout accordingly.
- Around line 104-121: Update the "should include mutableMessages in JSON
output" test to explicitly assert that the JSON result includes the
mutableMessages field and its expected value for the mutable-chat namespace:
after parsing stdout into result, locate the rule object for the namespace
created via mockNamespace({ id: "mutable-chat", persisted: true }) and add
expects that rule.mutableMessages is present and true (and optionally assert
mutableMessages is false or absent for the regular-chat rule); reference the
test name "should include mutableMessages in JSON output", the runCommand call,
and the mockNamespace usages to find where to add these assertions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro

Run ID: 2cc0db5b-8083-4362-8aaa-c098f8034d44

📥 Commits

Reviewing files that changed from the base of the PR and between f8ecbdc and 88e1a67.

📒 Files selected for processing (37)
  • .claude/skills/ably-codebase-review/SKILL.md
  • .claude/skills/ably-new-command/SKILL.md
  • .claude/skills/ably-review/SKILL.md
  • README.md
  • docs/Project-Structure.md
  • src/base-command.ts
  • src/commands/apps/channel-rules/create.ts
  • src/commands/apps/channel-rules/delete.ts
  • src/commands/apps/channel-rules/index.ts
  • src/commands/apps/channel-rules/list.ts
  • src/commands/apps/channel-rules/update.ts
  • src/commands/apps/index.ts
  • src/commands/apps/rules/create.ts
  • src/commands/apps/rules/delete.ts
  • src/commands/apps/rules/index.ts
  • src/commands/apps/rules/list.ts
  • src/commands/apps/rules/update.ts
  • src/commands/channel-rule/create.ts
  • src/commands/channel-rule/delete.ts
  • src/commands/channel-rule/index.ts
  • src/commands/channel-rule/list.ts
  • src/commands/channel-rule/update.ts
  • src/utils/channel-rule-display.ts
  • test/fixtures/control-api.ts
  • test/unit/commands/apps/channel-rules/create.test.ts
  • test/unit/commands/apps/channel-rules/delete.test.ts
  • test/unit/commands/apps/channel-rules/list.test.ts
  • test/unit/commands/apps/channel-rules/update.test.ts
  • test/unit/commands/apps/rules/create.test.ts
  • test/unit/commands/apps/rules/delete.test.ts
  • test/unit/commands/apps/rules/index.test.ts
  • test/unit/commands/apps/rules/list.test.ts
  • test/unit/commands/apps/rules/update.test.ts
  • test/unit/commands/channel-rule/create.test.ts
  • test/unit/commands/channel-rule/delete.test.ts
  • test/unit/commands/channel-rule/list.test.ts
  • test/unit/commands/channel-rule/update.test.ts
✅ Files skipped from review due to trivial changes (1)
  • docs/Project-Structure.md
🚧 Files skipped from review as they are similar to previous changes (12)
  • src/commands/apps/index.ts
  • src/commands/channel-rule/delete.ts
  • src/utils/channel-rule-display.ts
  • test/unit/commands/apps/rules/create.test.ts
  • test/unit/commands/channel-rule/create.test.ts
  • src/commands/apps/rules/create.ts
  • test/unit/commands/apps/rules/delete.test.ts
  • test/unit/commands/apps/rules/update.test.ts
  • src/commands/apps/channel-rules/delete.ts
  • src/commands/channel-rule/update.ts
  • test/unit/commands/channel-rule/update.test.ts
  • src/commands/channel-rule/create.ts

Copy link
Copy Markdown
Contributor

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be json error-handling related issue.
I was testing following command for existing mutable-messages rule on chat namespace

pnpm cli apps rules create --name "chat" --mutable-messages --json

The command produced the following output

Control API Request Error: {
  message: 'API request failed with status 400: Bad Request',
  method: 'POST',
  response: {
    message: 'Unable to modify existing channel namespace id with POST',
    code: 40300,
    statusCode: 400,
    href: 'https://help.ably.io/error/40300',
    details: null
  },
  statusCode: 400,
  url: 'https://control.ably.net/v1/apps/jy3uew/namespaces'
}
{"type":"error","command":"apps:rules:create","success":false,"error":"API request failed (400 Bad Request): Unable to modify existing channel namespace id with POST","appId":"jy3uew"}

A few issues I noticed:

  1. This does not appear to be valid JSON error output for --json. Same goes for --pretty-json, maybe fix across codebase needed.
  2. errorCode 40300 and helpUrl https://help.ably.io/error/40300 is not included in actual json error
  3. Not sure if we should include url field https://control.ably.net/v1/apps/jy3uew/namespaces in the json error message.

@sacOO7
Copy link
Copy Markdown
Contributor

sacOO7 commented Mar 12, 2026

Since, legacy command paths (apps channel-rules and channel-rule) are aliases that silently delegate to apps rules, will be great to have a deprecation warning on stderr would give users time to migrate their scripts.

Copy link
Copy Markdown
Contributor

@sacOO7 sacOO7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@umair-ably umair-ably merged commit fe798dc into main Mar 12, 2026
10 of 11 checks passed
@umair-ably umair-ably deleted the DX-264-rules branch March 12, 2026 15:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

3 participants