镜像站点 · 本页由第三方 GitHub 只读镜像提供,非 GitHub 官方站点,不接受任何登录或凭据输入。前往 github.com
Skip to content

feat(cli): introduce recordings commands - #98

Merged
bespoyasov merged 15 commits into
mainfrom
feat/recordings-commands
Oct 5, 2026
Merged

bespoyasov merged 15 commits into
mainfrom
feat/recordings-commands

Conversation

@bespoyasov

Copy link
Copy Markdown
Collaborator
  • Add recording management commands to the CLI (policy, rule, targeting-key) backed by MCP tools on the confidence-flags server
  • Extract shared utilities (requireAuth, readJsonFile, serverOpts, test helpers) to reduce duplication across events and recordings

Commands Added

Command Description
confidence recordings policy list List recording policies
confidence recordings policy create Create a recording policy
confidence recordings policy get <policy> Get recording policy details and rules
confidence recordings rule add Add a recording rule to a policy
confidence recordings rule enable <rule> Enable a recording rule
confidence recordings rule disable <rule> Disable a recording rule
confidence recordings targeting-key show <client> Show context schema and targeting keys
confidence recordings targeting-key add Add a context field as targeting key

All write commands (policy create, rule add, targeting-key add) support --from-file for JSON input.

…getFile

Fix handleMcpError bug where extractText threw before custom error
formatting could run. Extract withAuth higher-order wrapper and
resolveTargetFile helper to eliminate repeated boilerplate across
13 handlers. Remove redundant ?? fallbacks (yargs provides defaults),
switch to barrel imports from @network/index, and simplify
addRecordingRule param forwarding.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@bespoyasov
bespoyasov marked this pull request as ready for review October 5, 2026 09:58

@filipmyllari filipmyllari left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review notes (AI-assisted). Inline comments below. Tests were skimmed and the events refactor was only checked for the withAuth/safely mapping.

Comment on lines +43 to +46
message('');
message('Rules:');
print({
data: data.rules.map((r) => ({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With --json / --output json, this prints one JSON document, then the plain-text line Rules: (or No rules configured.), then a second JSON document. The combined output isn't valid JSON, so policy get X --json | jq fails. In JSON mode, print a single object ({...policy, rules}) and skip the message calls.

Comment on lines +47 to +48
if (data.nextPageToken) {
message(`\nNext page: --page-token ${data.nextPageToken}`);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

In JSON mode, the Next page: --page-token … line is added after the JSON array, so the output isn't valid JSON. Include nextPageToken in the JSON output, or write the hint to stderr.

}

print<PolicyRow>({
data: data.recordingPolicies.map((p) => ({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Proto3 JSON drops empty repeated fields, so recordingPolicies and clients may be missing. If they are, an account with no policies, or a policy with no clients, hits Cannot read properties of undefined instead of the empty-state message. Use ?? [] here, and the same in get-policy.ts:33 (clients) and show-targeting-keys.ts:31 (fields).

};

export const addRule = withAuth(async function addRule(argv, token) {
const params = resolveTargetFile<RuleParams>(argv, () => ({

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With --from-file, the parsed file is used as-is, with no checks. Missing keys are sent as undefined and the CLI defaults are lost (audience-percentage 100, sample-rate 1, enabled true). The file also has to use kebab-case keys such as "targeting-key", which nothing documents. Merge the file over the argv defaults and check the required keys after the merge. The same applies to add-targeting-key.ts and create-policy.ts.

Comment on lines +97 to +107
.option('audience-percentage', {
type: 'number',
describe: 'Stable audience percentage (0-100)',
default: 100,
})
.option('sample-rate', {
type: 'number',
describe: 'Session sample rate (0-1)',
default: 1,
})
.option('enabled', {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nothing checks the range of --audience-percentage (0–100) or --sample-rate (0–1). A value that isn't a number becomes NaN and is sent anyway, and --sample-rate 50 goes through as is. Add these checks to the existing .check(). Also, enabled defaults to true with a 100% audience, so leaving the flags off starts recording every visitor straight away. Consider defaulting to enabled: false.

'is-entity': argv['is-entity'] as boolean,
}));

const result = await addContextField(token, {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

addContextField takes no client argument, but targeting-key show is per-client. I couldn't check the MCP tool's schema. If the field belongs to one client's schema, a --client option is missing.

import { extractText, type CallToolResult } from '@spotify-confidence/core';
import { error, fail } from '@output/print.js';

export function handleMcpError(result: CallToolResult): boolean {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

handleMcpError, which prints the "Available clients are:" list nicely, is only used by list, get and show. policy create --client-name is the command most likely to return that error, but it goes through printMcpResult → safely and prints the raw message. Use handleMcpError there as well.

Comment thread packages/cli/src/network/recordings.ts Outdated
return mcpCallTool(serverOpts(token), 'addContextField', {
fieldName: params.fieldName,
fieldType: params.fieldType,
isEntity: String(params.isEntity),

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

String(params.isEntity) sends the string "true" instead of a boolean. That's fine if the tool's schema expects a string, but it's worth confirming.

bespoyasov and others added 8 commits October 5, 2026 13:27
In JSON mode, policy details and rules were printed as two separate JSON
documents with plain-text lines between them, breaking piped jq usage.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The next-page hint was appended as plain text after the JSON array,
breaking piped jq usage. In JSON mode, nextPageToken is now included
in the envelope's meta field instead.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Proto3 JSON omits empty repeated fields, so recordingPolicies, clients,
and fields may be undefined. Default to empty arrays to avoid crashing.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…keys

With --from-file, the parsed file replaced argv entirely, losing CLI
defaults (audience-percentage 100, sample-rate 1, enabled true) and
requiring undocumented kebab-case keys. Now the file is merged over
defaults with camelCase-to-kebab normalization, and required keys are
validated post-merge.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Reject out-of-range, NaN, and Infinity values for --audience-percentage
(0-100) and --sample-rate (0-1) in both CLI args and --from-file input.
Default enabled to false to avoid accidentally recording all visitors.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rgeting-key add

The MCP tool accepts displayName, entityReference, and a clients list,
but the CLI only sent fieldName, fieldType, and isEntity. Wire through
the missing parameters. Update tests for earlier default changes.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Add formatted MCP error handling to create-policy, add-rule,
add-targeting-key, and set-rule-enabled. Rename handleMcpError to
tryHandleMcpError for clarity.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The MCP tool expects a boolean for isEntity. The String() conversion
sent "true"/"false" strings instead.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@bespoyasov
bespoyasov merged commit d83032f into main Oct 5, 2026
6 checks passed
@bespoyasov
bespoyasov deleted the feat/recordings-commands branch October 5, 2026 11:55
@github-actions github-actions Bot mentioned this pull request Oct 5, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants