feat: ABSmartly CLI TypeScript implementation

[joalves]

Summary

• Complete TypeScript port of ABSmartly CLI
• 63 test files, 612 tests, 100% command coverage
• Full API client with MSW mock infrastructure

Test plan

• npm ci && npx vitest run — all tests pass
• npx tsc --noEmit — compiles clean

Summary by CodeRabbit

• New Features

  • Full ABSmartly CLI added with commands for experiments (create, list, get, search, activity, schedule, full-on, restart, start/stop, archive, development), apps, envs, metrics, goals, segments, teams, users, roles, permissions, API keys, webhooks, flags, tags and utilities (auth, config, doctor, open, completion, raw API).

• Documentation

  • Added README, LICENSE and Prettier config; package metadata and publish settings included.

• Tests

  • Extensive unit and integration test suites for CLI commands and API client.

[coderabbitai]

Walkthrough

This pull request adds a full TypeScript CLI project: packaging and config files (.gitignore, .npmignore, package.json, tsconfig, Prettier, Vitest config, LICENSE, README), a comprehensive Axios-based API client (APIClient, createAPIClient) with extensive endpoints and error handling, branded typed IDs and validators, many Commander.js command modules (experiments, auth, config, apps, api-keys, etc.), numerous unit and integration tests (Vitest, MSW), test helpers and live-test tooling, and CLI bootstrap code wiring all commands.

Estimated code review effort

🎯 5 (Critical) | ⏱️ ~150 minutes

Poem

🐇 I hopped through code with eager paws,
New commands, types and helpful laws,
Clients and tests in tidy rows,
Branded IDs and validator bows,
A CLI garden — nibble, then applause!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 3.70% 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 summarizes the primary change: a complete TypeScript implementation of the ABSmartly CLI with comprehensive test coverage.

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

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch cli-source

---

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

[coderabbitai]

Actionable comments posted: 2

Note

Due to the large number of review comments, Critical severity comments were prioritized as inline comments.

🟠 Major comments (27)

src/commands/experiments/generate-template.ts-10-20 (1)

10-20: ⚠️ Potential issue | 🟠 Major

Keep the generated template valid out of the box.

src/lib/template/generator.ts:14-18 already assumes a machine-safe name (my_experiment) and derives display_name from it. Overriding that with My Experiment here means the no-flag path emits a name with spaces, and --type still accepts arbitrary strings despite the help text only allowing test or feature. That makes it easy to generate a template the follow-up create flow rejects.

Proposed fix

 export const generateTemplateCommand = new Command('generate-template')
   .aliases(['gen-template', 'template'])
   .description('Generate a sample experiment markdown template')
-  .option('--name <name>', 'experiment name for the template', 'My Experiment')
+  .option('--name <name>', 'experiment name for the template', 'my_experiment')
   .option('--type <type>', 'experiment type (test or feature)', 'test')
   .option('-o, --output <path>', 'output file path (defaults to stdout)')
   .action(withErrorHandling(async (options) => {
     const globalOptions = getGlobalOptions(generateTemplateCommand);
     const client = await getAPIClientFromOptions(globalOptions);
+    const type = String(options.type).trim();
+
+    if (type !== 'test' && type !== 'feature') {
+      throw new Error("--type must be either 'test' or 'feature'");
+    }

     const content = await generateTemplate(client, {
-      name: options.name,
-      type: options.type,
+      name: String(options.name).trim().replace(/\s+/g, '_'),
+      type,
     });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/experiments/generate-template.ts` around lines 10 - 20, The CLI
currently passes raw options.name ("My Experiment") and unconstrained
options.type into generateTemplate, causing invalid machine names and arbitrary
types; change the default for --name to a machine-safe string (e.g.,
"my_experiment") and normalize/validate options.name and options.type before
calling generateTemplate (allow only 'test' or 'feature', map/trim/canonicalize
input), so the payload sent to generateTemplate matches the expectations in
src/lib/template/generator.ts; update the option defaults and add a small
normalization/validation step in the .action handler that uses
generateTemplateCommand, options.name and options.type to produce the sanitized
values passed to generateTemplate.

PROJECT_STATUS.md-11-12 (1)

11-12: ⚠️ Potential issue | 🟠 Major

Update test count to reflect actual current state.

PROJECT_STATUS.md states "All 6 tests passing", but the project currently has 601 individual test cases across 63 test files. The document reflects an earlier project phase and should be updated to "All 601+ tests passing" or similar to accurately represent the current test coverage.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@PROJECT_STATUS.md` around lines 11 - 12, Update the test summary line in
PROJECT_STATUS.md: replace the string "✅ **Tests**: All 6 tests passing" with an
accurate count such as "✅ **Tests**: All 601+ tests passing" (or similar wording
indicating 601+ tests) so the document reflects the current total of 601 test
cases across 63 files.

PORTING_SUMMARY.md-5-15 (1)

5-15: ⚠️ Potential issue | 🟠 Major

This reads like an old progress note, not the state of this PR.

The file says only 6 tests are passing and that many core commands are still “to be completed”, but the current PR objectives describe a complete port with 63 test files and full command coverage. Please either refresh this to the current state or mark it clearly as historical progress documentation.

Also applies to: 83-89, 90-143, 252-260

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@PORTING_SUMMARY.md` around lines 5 - 15, The PORTING_SUMMARY.md currently
contains outdated progress notes (e.g., the "✅ Completed" section and the lines
describing only 6 tests passing and incomplete commands); update this file to
either reflect the current PR state (all port objectives, 63 test files, full
command coverage, updated test pass counts) or explicitly mark the existing
content as historical progress notes. Locate the "✅ Completed" heading and the
sections referencing test counts and "to be completed" commands and replace or
annotate them with the accurate summary for this PR (or add a "Historical
progress" subsection) so the document aligns with the PR objectives.

ULTIMATE_FINAL_STATUS.md-9-33 (1)

9-33: ⚠️ Potential issue | 🟠 Major

These “final” status metrics are already stale.

The document hard-codes 421 tests across 24 test files, while the current PR objectives say 612 tests across 63 files. Because the deployment recommendation is built on those numbers, this status report is no longer reliable as written.

Also applies to: 145-152, 258-289

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ULTIMATE_FINAL_STATUS.md` around lines 9 - 33, The Final Metrics block in
ULTIMATE_FINAL_STATUS.md contains stale hard-coded test and file counts (e.g.,
"421 tests / 24 test files"); update these entries to reflect the current PR
targets (612 tests across 63 files) or, better, replace the hard-coded values
with dynamic placeholders populated from CI/test-run artifacts; locate the
"Final Metrics" ASCII table and the other occurrences noted in the comment and
change the Test Files/Total Tests fields (and any duplicated blocks) to either
the updated counts or to variables that are injected during the release/CI step
so the document stays accurate.

CODE_REVIEW_FINDINGS.md-5-12 (1)

5-12: ⚠️ Potential issue | 🟠 Major

Refresh these coverage findings before merging.

This report still says there are 10 test files / 114 tests, zero command tests, and 40 untested command files, but the current PR objectives say 63 test files, 612 tests, and 100% command coverage. As written, the document gives a materially wrong picture of test readiness.

Also applies to: 102-124, 178-184

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@CODE_REVIEW_FINDINGS.md` around lines 5 - 12, The summary numbers in the
"Test Coverage Analysis" and related report sections (e.g., the "Test Coverage
Analysis" heading and the lines showing "10 test files / 114 tests", "zero
command tests", and "40 untested command files") are stale and conflict with the
PR objectives; update CODE_REVIEW_FINDINGS.md to reflect the current PR metrics
(63 test files, 612 tests, 100% command coverage) and ensure the same corrected
figures are applied consistently in the other mentioned sections (lines
referenced around 102-124 and 178-184 and the "Test Coverage Analysis" block) so
the document no longer presents a materially incorrect picture of test
readiness.

TEST_COVERAGE_SUMMARY.md-3-10 (1)

3-10: ⚠️ Potential issue | 🟠 Major

Update the baseline before publishing this coverage summary.

This file still says there are 385 tests, 22 test files, and only 3 of 40 commands covered, but the PR objectives say 63 test files, 612 tests, and full command coverage. The blocker recommendation at the end is therefore based on stale numbers.

Also applies to: 41-60, 161-176

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@TEST_COVERAGE_SUMMARY.md` around lines 3 - 10, Update the stale coverage
summary values on the "Status:" line and the related bullets: replace "385
tests, 22 test files, 85% unit coverage, 7.5% integration coverage" with the PR
baseline numbers ("612 tests, 63 test files, [update unit/integration
percentages if available] and full command coverage"), update the "Integration
Tests" bullet from "POOR (3 of 40 commands tested)" to reflect full command
coverage (e.g., "FULL (40 of 40 commands tested)" or the accurate counts from
the PR), and adjust the "Critical Risk" or blocker wording to match the new
baseline; edit the lines that include "Status:", the "- **Integration Tests:**"
bullet, and the "Critical Risk:" bullet so the summary reflects the PR
objectives rather than the stale numbers.

ABSOLUTE_FINAL_SUMMARY.md-11-22 (1)

11-22: ⚠️ Potential issue | 🟠 Major

Synchronise these headline figures with the actual PR state.

This summary still reports 23 test files, 402 tests, and ~87% coverage, whereas the current PR objectives say 63 test files, 612 tests, and full command coverage. Since the rest of the document builds on those figures, the whole “absolute final” status becomes misleading.

Also applies to: 91-101, 196-209, 399-418

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ABSOLUTE_FINAL_SUMMARY.md` around lines 11 - 22, Update the headline metrics
in ABSOLUTE_FINAL_SUMMARY.md's summary table and any repeated sections so they
match the current PR state: change "Test Files" to 63, "Total Tests" to 612,
ensure coverage/command‑coverage reads as full (100% or "full command coverage"
consistent with PR goals), and adjust any derived figures (percentages/totals)
that rely on these values; then scan the rest of the document for other
occurrences of those metrics and make them consistent so the table and all
narrative sections report identical, up-to-date numbers.

src/commands/auth/index.ts-8-15 (1)

8-15: ⚠️ Potential issue | 🟠 Major

Avoid exposing API keys via argv.

Making --api-key mandatory leaks the secret into shell history and, on shared systems, process listings. Please support stdin, an interactive prompt, or an environment variable for the credential instead of requiring it on the command line.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/auth/index.ts` around lines 8 - 15, The command currently
requires the API key via .requiredOption('--api-key <key>') on loginCommand
which leaks secrets; make the --api-key option optional and implement a secure
fallback inside the action handler (the async callback wrapped by
withErrorHandling) that: 1) first checks process.env.ABSMARTLY_API_KEY, 2) if
not present, prompts the user securely for the key via stdin/interactive prompt
(e.g., readline or a silent prompt) and does not echo it, and 3) only if a key
is obtained proceeds; update validation logic in the action callback to use this
resolved key and error out with a clear message if still missing, and ensure the
key is never logged or printed.

src/commands/completion/index.ts-3-16 (1)

3-16: ⚠️ Potential issue | 🟠 Major

Do not report success for a completion script you do not generate.

This command accepts any shell name, prints advisory comments, and exits 0, so abs completion bash appears to succeed even though there is nothing a shell can source. Either generate a real script here, or fail clearly / rename the command so it is obviously informational rather than functional.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/completion/index.ts` around lines 3 - 16, The completionCommand
currently accepts any shell and exits 0 while only printing advisory text;
update the completionCommand action to either (A) generate and print a valid
completion script for supported shells (limit to
'bash','zsh','fish','powershell') by detecting the shell argument and emitting
the appropriate script template, or (B) make it explicitly informational by
renaming the command (e.g., to completion-info) and changing the description to
state "does not generate scripts", or if you keep the name, validate the shell
and exit non‑zero with a clear error message when an unsupported/empty shell is
provided; modify the action handler function attached to completionCommand
accordingly and ensure exit codes reflect success only when a real script was
emitted.

package.json-73-73 (1)

73-73: ⚠️ Potential issue | 🟠 Major

Remove the sibling file: dependency that cannot be resolved.

The devDependency absmartly-api-mocks references file:../absmartly-api-mocks, but that package does not exist in the repository structure. This causes npm ci to fail, breaking the stated test plan. Publish the package, vendor it inside the repo, or remove the dependency from the shared manifest.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@package.json` at line 73, The package.json contains an unresolved sibling
devDependency "absmartly-api-mocks": "file:../absmartly-api-mocks" which breaks
npm ci; remove that file: entry from devDependencies in package.json (or replace
it with a published version string or a vendored local path that actually
exists) and then regenerate lockfile (npm install / npm ci) to ensure the
manifest and lockfile are consistent; locate the "absmartly-api-mocks" entry in
package.json to make the change.

src/commands/auth/index.ts-15-31 (1)

15-31: ⚠️ Potential issue | 🟠 Major

Make login rollback on partial persistence failures.

This action writes the keyring entry and then writes the profile. If setProfile() throws, the API key remains stored but the profile is missing, which leaves the CLI partially configured. Wrap both writes in a small transaction and remove the keyring entry on rollback when the second step fails.

Possible rollback pattern

 .action(withErrorHandling(async (options) => {
   const profileName = options.profile || 'default';
-
-  await setAPIKey(options.apiKey, profileName);
 
   const profile = {
     api: {
       endpoint: options.endpoint,
@@
     application: options.app,
     environment: options.env,
   };
-
-  setProfile(profileName, profile);
+  let apiKeySaved = false;
+  try {
+    await setAPIKey(options.apiKey, profileName);
+    apiKeySaved = true;
+    setProfile(profileName, profile);
+  } catch (error) {
+    if (apiKeySaved) {
+      await deleteAPIKey(profileName).catch(() => undefined);
+    }
+    throw error;
+  }
 
   console.log(`✓ Logged in successfully (profile: ${profileName})`);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/auth/index.ts` around lines 15 - 31, Wrap the two persistence
calls in a transactional pattern: after calling setAPIKey(profileName) and
before returning, call setProfile(profileName, profile) inside the same try
block; if setProfile throws, roll back by deleting the keyring entry (call the
appropriate removal function such as removeAPIKey(profileName) or
deleteAPIKey(profileName)) in the catch and rethrow the error so the caller sees
the failure. Implement this flow in the async action (the function passed to
withErrorHandling) around the existing setAPIKey and setProfile calls so a
failed setProfile does not leave the API key persisted alone.

src/commands/auth/index.ts-47-63 (1)

47-63: ⚠️ Potential issue | 🟠 Major

Do not collapse every failure into “Not authenticated”.

This catch block also swallows real operational errors from getAPIKey() and getProfile(), such as keyring access failures, and rewrites them as an auth miss. That sends users to abs auth login even when the stored credentials are fine. Please only intercept the expected “profile/key missing” cases here and rethrow the rest.

Narrow the catch to genuine unauthenticated cases

-    } catch (error) {
-      console.error('Not authenticated. Run `abs auth login` to authenticate.');
-      process.exit(1);
-    }
+    } catch (error) {
+      if (error instanceof Error && /^Profile ".+" not found$/.test(error.message)) {
+        console.error('Not authenticated. Run `abs auth login` to authenticate.');
+        process.exitCode = 1;
+        return;
+      }
+      throw error;
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/auth/index.ts` around lines 47 - 63, The catch currently masks
all errors from getProfile and getAPIKey as an unauthenticated state; change the
logic so only genuine "profile or key missing" cases are handled locally and all
other errors are rethrown. Concretely, call getProfile(profileName) and await
getAPIKey(profileName) inside try, then if profile is falsy or apiKey is
explicitly missing handle the unauthenticated message; in the catch block
detect/allow only known "not found" or "no such profile/key" error variants (or
check the returned values before throwing) and rethrow any other error so
operational failures from getProfile/getAPIKey bubble up instead of always
printing the auth suggestion.

CRITICAL_FIXES_NEEDED.md-241-250 (1)

241-250: ⚠️ Potential issue | 🟠 Major

The rollback example can still leave partial state while claiming a full rollback.

If deleteAPIKey('default') fails, that error is swallowed and the code still throws No changes were made. At that point a credential may still be persisted, so the message overstates the rollback guarantee and can mislead the operator during recovery.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@CRITICAL_FIXES_NEEDED.md` around lines 241 - 250, The rollback currently
swallows errors from deleteAPIKey('default') and still throws "No changes were
made", which falsely claims a full rollback; modify the catch block that handles
saveError so it captures any error from deleteAPIKey (e.g., assign to
rollbackError), do not swallow it, and throw a combined Error that includes both
the original saveError (saveError.message) and the rollback/cleanup error
(rollbackError.message) and explicitly state that the rollback may be partial;
reference the existing deleteAPIKey and saveError symbols and ensure the thrown
message accurately reflects partial rollback and includes both error details for
operator debugging.

ULTRA_DEEP_ERROR_AUDIT_FINAL.md-40-40 (1)

40-40: ⚠️ Potential issue | 🟠 Major

Replace workstation-specific absolute paths with repo-relative paths.

This report hard-codes /Users/joalves/... locations throughout. That leaks contributor-specific path information and makes the references unusable for anyone cloning the repo elsewhere. Please switch these to repo-relative paths such as src/commands/generate/index.ts.

Also applies to: 110-110, 504-529

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@ULTRA_DEEP_ERROR_AUDIT_FINAL.md` at line 40, The audit contains hard-coded
workstation absolute paths (strings beginning with "/Users/") that must be
replaced with repo-relative paths; search the markdown for any occurrences of
the "/Users/" prefix and replace each with the correct repository-relative
reference, ensuring the target file references are expressed relative to the
repo root (not user-specific), and update all listed sections/occurrences
consistently.

CRITICAL_FIXES_NEEDED.md-101-136 (1)

101-136: ⚠️ Potential issue | 🟠 Major

The timeout check does not prevent a regex hang.

Date.now() is checked only after variantPattern.exec(content) completes. If the regex performs catastrophic backtracking, the synchronous call to exec() can block the entire event loop indefinitely, preventing the timeout check from ever running. Fixing this requires either: a non-pathological regex pattern (e.g., avoiding nested quantifiers), a non-regex parser, or running the regex in a separate thread with proper time limits.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@CRITICAL_FIXES_NEEDED.md` around lines 101 - 136, The parseVariants function
currently relies on a potentially pathological regex (variantPattern) and only
checks Date.now() after variantPattern.exec(content) returns, so a catastrophic
backtrack can block the event loop and bypass the TIMEOUT_MS/MAX_ITERATIONS
guards; fix it by removing the risky nested quantifiers in variantPattern or
replace the regex-based parser with a deterministic parser (e.g.,
line-by-line/state machine using string methods) OR move the regex work into a
worker/child process and enforce a hard timeout/kill there; ensure you update
parseVariants (and any callers) to use the safe parser or the worker API and
keep the existing TIMEOUT_MS/MAX_ITERATIONS semantics as a secondary safeguard.

CRITICAL_FIXES_NEEDED.md-23-45 (1)

23-45: ⚠️ Potential issue | 🟠 Major

tempPath is out of scope in the cleanup path.

The tempPath variable is declared with const inside the try block and then referenced in the catch block. Since const is block-scoped and try and catch are separate blocks, this code would throw a ReferenceError and the temporary file cleanup would never execute.

Declare tempPath outside the try block and assign it inside:

Suggested correction

 export function saveConfig(config: Config): void {
+  let tempPath: string | undefined;
   try {
     ensureConfigDir();
     const path = getConfigPath();
-    const tempPath = path + '.tmp';
+    tempPath = path + '.tmp';
     const content = yaml.dump(config, { indent: 2, lineWidth: 120 });
@@
   } catch (error) {
     // Clean up temp file on error
     try {
-      if (existsSync(tempPath)) {
+      if (tempPath && existsSync(tempPath)) {
         unlinkSync(tempPath);
       }
     } catch {}

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@CRITICAL_FIXES_NEEDED.md` around lines 23 - 45, The temporary file path
tempPath is declared inside the try block in saveConfig, so the catch handler
cannot access it; move the declaration of tempPath (e.g., let tempPath: string |
undefined) above the try, assign it inside the try after computing path =
getConfigPath(), and then use that outer-scoped tempPath in the catch cleanup;
ensure functions referenced (ensureConfigDir, getConfigPath, writeFileSync,
renameSync, existsSync, unlinkSync) remain unchanged and only the tempPath scope
is adjusted so the cleanup logic can safely check and remove the temp file on
error.

docs/plans/2026-03-03-api-mocks-integration-design.md-241-247 (1)

241-247: ⚠️ Potential issue | 🟠 Major

Deleting the local shims breaks the “imports stay unchanged” migration.

The rest of the design says existing test imports should keep working via rewritten src/test/mocks/server.ts and src/test/mocks/error-handlers.ts. Removing those files instead would either break the current paths or force churn across the suite.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/plans/2026-03-03-api-mocks-integration-design.md` around lines 241 -
247, The deleted local shim files break the "imports stay unchanged" migration;
restore or recreate the deleted modules so existing imports continue to work:
re-add src/test/mocks/server.ts and src/test/mocks/error-handlers.ts as
compatibility shims that re-export or forward to the new unified mock
implementation, and ensure src/test/mocks/handlers.ts and
src/test/mocks/factories.ts are either restored or have thin forwarding modules
that import from the new implementation and export the same symbols so no test
import paths change.

TEST_REPORT.md-5-10 (1)

5-10: ⚠️ Potential issue | 🟠 Major

This report is stale and the counts do not reconcile.

The PR objective says this branch has 63 test files and 612 tests, while this report still says 114 tests / 10 files / 8 suites. It is also internally inconsistent: the per-file sections add up to 114, but the module table claims 90 API Client tests, which pushes the table total to 129.

Also applies to: 120-127, 229-234

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@TEST_REPORT.md` around lines 5 - 10, Update TEST_REPORT.md so the summary and
detailed sections reflect the current run (63 test files and 612 tests) and fix
the inconsistent module table totals; specifically update the "Total Tests",
"Test Files", "Test Suites", "Execution Time", and "Pass Rate" fields and
reconcile the per-file sections and the module table (the "API Client" row that
lists 90 tests) so their sums match the overall total; ensure any aggregate rows
(module table totals and per-file subtotal) compute from the listed per-file
counts rather than hard-coded values.

TYPE_DESIGN_ANALYSIS_PART2.md-341-347 (1)

341-347: ⚠️ Potential issue | 🟠 Major

The proposed numeric branch still drops the Unix epoch.

'0' enters this branch as a valid integer (satisfies asNumber >= 0), but the inner if (asNumber > 0) condition prevents its return, causing the code to fall through to string date parsing. This reintroduces the exact edge case this section aims to handle.

Suggested correction

   const asNumber = Number(dateStr);
   if (!isNaN(asNumber) && Number.isInteger(asNumber) && asNumber >= 0) {
-    // Check if it's a reasonable timestamp
-    if (asNumber > 0) {
-      return TimestampMs(asNumber);
-    }
+    return TimestampMs(asNumber);
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@TYPE_DESIGN_ANALYSIS_PART2.md` around lines 341 - 347, The numeric-branch
incorrectly rejects the Unix epoch '0' by checking if asNumber > 0 before
returning TimestampMs(asNumber); update the logic around asNumber (derived from
Number(dateStr)) so that when asNumber is a non-negative integer it returns
TimestampMs(asNumber) including 0 (e.g., change the inner condition to allow
asNumber === 0 or use asNumber >= 0 consistently), ensuring dateStr '0' is
handled by the numeric branch rather than falling through to string parsing.

src/commands/doctor/index.ts-15-25 (1)

15-25: ⚠️ Potential issue | 🟠 Major

Make doctor honour the same global connection overrides as the other commands.

Lines 19-25 and 34-38 always resolve the default profile, keyring entry, and configured endpoint, so abs doctor --profile staging, --endpoint ..., or --api-key ... will still diagnose the default profile instead of the requested target. Please derive profileName, endpoint, and apiKey from getGlobalOptions(doctorCommand) before creating the client.

Possible fix

 import { Command } from 'commander';
 import chalk from 'chalk';
 import { getProfile, loadConfig } from '../../lib/config/config.js';
 import { getAPIKey } from '../../lib/config/keyring.js';
 import { createAPIClient } from '../../lib/api/client.js';
-import { withErrorHandling } from '../../lib/utils/api-helper.js';
+import { getGlobalOptions, withErrorHandling } from '../../lib/utils/api-helper.js';

 export const doctorCommand = new Command('doctor')
   .description('Diagnose configuration issues')
   .action(withErrorHandling(async () => {
+    const globalOptions = getGlobalOptions(doctorCommand);
     console.log(chalk.bold('\n🔍 ABSmartly CLI Diagnostics\n'));

     let allGood = true;

     try {
       const config = loadConfig();
       console.log(chalk.green('✓') + ' Configuration file loaded');

-      const profileName = config['default-profile'];
+      const profileName = (globalOptions.profile as string) || config['default-profile'];
       console.log(chalk.green('✓') + ` Default profile: ${profileName}`);

       const profile = getProfile(profileName);
       console.log(chalk.green('✓') + ` Profile found: ${profileName}`);

-      const apiKey = await getAPIKey(profileName);
+      const apiKey = (globalOptions.apiKey as string) || await getAPIKey(profileName);
+      const endpoint = (globalOptions.endpoint as string) || profile.api.endpoint;
       if (!apiKey) {
         console.log(chalk.red('✗') + ' API key not found in keyring');
         console.log(chalk.yellow('  Run: abs auth login --api-key YOUR_KEY'));
         allGood = false;
       } else {
         console.log(chalk.green('✓') + ' API key found');
       }

-      console.log(chalk.green('✓') + ` API endpoint: ${profile.api.endpoint}`);
+      console.log(chalk.green('✓') + ` API endpoint: ${endpoint}`);

       if (apiKey) {
         console.log(chalk.blue('\nℹ Testing API connectivity...'));
-        const client = createAPIClient(profile.api.endpoint, apiKey);
+        const client = createAPIClient(endpoint, apiKey, { verbose: globalOptions.verbose });

         try {
           await client.listApplications();

Also applies to: 34-38

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/doctor/index.ts` around lines 15 - 25, The doctor command
currently always reads defaults via loadConfig/getProfile/getAPIKey; change it
to first call getGlobalOptions(doctorCommand) and use the returned overrides to
derive profileName, endpoint and apiKey (falling back to
config['default-profile'] if none provided) before calling getProfile and
getAPIKey and before creating the client so flags like --profile, --endpoint and
--api-key are honored; update usage locations around where profileName, endpoint
and apiKey are computed (references: loadConfig, getGlobalOptions,
doctorCommand, getProfile, getAPIKey) to use the values from
getGlobalOptions(doctorCommand) when present.

src/commands/experiments/list.ts-24-35 (1)

24-35: ⚠️ Potential issue | 🟠 Major

Wrap parseInt in a single-argument parser to avoid radix misinterpretation.

Commander calls custom parsers as (value, previous). With a default value like 20 on --limit, this becomes parseInt('10', 20), treating 20 as the radix parameter and producing incorrect results. Use a wrapper such as value => Number.parseInt(value, 10) instead of passing parseInt directly.

Suggested fix

+const parseInteger = (value: string): number => Number.parseInt(value, 10);
+
 export const listCommand = new Command('list')
   .description('List experiments')
   .option('--state <state>', 'filter by state (created, ready, running, stopped, archived)')
   .option('--type <type>', 'filter by type (test, feature)')
   .option('--app <app>', 'filter by application')
   .option('--search <query>', 'search by name or display name')
   .option('--unit-types <ids>', 'filter by unit types (comma-separated IDs)')
   .option('--owners <ids>', 'filter by owner user IDs (comma-separated)')
   .option('--teams <ids>', 'filter by team IDs (comma-separated)')
   .option('--tags <ids>', 'filter by tag IDs (comma-separated)')
   .option('--created-after <timestamp>', 'filter experiments created after timestamp')
   .option('--created-before <timestamp>', 'filter experiments created before timestamp')
   .option('--started-after <timestamp>', 'filter experiments started after timestamp')
   .option('--started-before <timestamp>', 'filter experiments started before timestamp')
   .option('--stopped-after <timestamp>', 'filter experiments stopped after timestamp')
   .option('--stopped-before <timestamp>', 'filter experiments stopped before timestamp')
   .option('--analysis-type <type>', 'filter by analysis type (fixed_horizon, group_sequential)')
   .option('--running-type <type>', 'filter by running type (full_on, experiment)')
-  .option('--alert-srm <value>', 'filter by sample ratio mismatch alert (1 for true)', parseInt)
-  .option('--alert-cleanup-needed <value>', 'filter by cleanup needed alert (1 for true)', parseInt)
-  .option('--alert-audience-mismatch <value>', 'filter by audience mismatch alert (1 for true)', parseInt)
-  .option('--alert-sample-size-reached <value>', 'filter by sample size reached alert (1 for true)', parseInt)
-  .option('--alert-experiments-interact <value>', 'filter by experiments interact alert (1 for true)', parseInt)
-  .option('--alert-group-sequential-updated <value>', 'filter by group sequential updated alert (1 for true)', parseInt)
-  .option('--alert-assignment-conflict <value>', 'filter by assignment conflict alert (1 for true)', parseInt)
-  .option('--alert-metric-threshold-reached <value>', 'filter by metric threshold reached alert (1 for true)', parseInt)
+  .option('--alert-srm <value>', 'filter by sample ratio mismatch alert (1 for true)', parseInteger)
+  .option('--alert-cleanup-needed <value>', 'filter by cleanup needed alert (1 for true)', parseInteger)
+  .option('--alert-audience-mismatch <value>', 'filter by audience mismatch alert (1 for true)', parseInteger)
+  .option('--alert-sample-size-reached <value>', 'filter by sample size reached alert (1 for true)', parseInteger)
+  .option('--alert-experiments-interact <value>', 'filter by experiments interact alert (1 for true)', parseInteger)
+  .option('--alert-group-sequential-updated <value>', 'filter by group sequential updated alert (1 for true)', parseInteger)
+  .option('--alert-assignment-conflict <value>', 'filter by assignment conflict alert (1 for true)', parseInteger)
+  .option('--alert-metric-threshold-reached <value>', 'filter by metric threshold reached alert (1 for true)', parseInteger)
   .option('--significance <value>', 'filter by significance (positive, negative, insignificant)')
-  .option('--limit <number>', 'maximum number of results', parseInt, 20)
-  .option('--offset <number>', 'offset for pagination', parseInt, 0)
-  .option('--page <number>', 'page number for pagination', parseInt)
+  .option('--limit <number>', 'maximum number of results', parseInteger, 20)
+  .option('--offset <number>', 'offset for pagination', parseInteger, 0)
+  .option('--page <number>', 'page number for pagination', parseInteger)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/experiments/list.ts` around lines 24 - 35, Commander invokes
custom parsers as (value, previous), so passing raw parseInt causes radix to be
misinterpreted when a default is provided; update all option parsers that
currently pass parseInt (e.g., the options '--alert-srm',
'--alert-cleanup-needed', '--alert-audience-mismatch',
'--alert-sample-size-reached', '--alert-experiments-interact',
'--alert-group-sequential-updated', '--alert-assignment-conflict',
'--alert-metric-threshold-reached', '--limit', '--offset', '--page') to use a
single-argument wrapper like value => Number.parseInt(value, 10) (or v =>
Number(v) where appropriate) so the parser signature matches Commander and the
radix is fixed.

src/commands/config/index.ts-14-95 (1)

14-95: ⚠️ Potential issue | 🟠 Major

Wrap these actions with the shared error handler.

The config helpers invoked here already throw for invalid YAML, missing profiles, invalid boolean values, permission errors, and so on. Because these actions are not using withErrorHandling, those failures will bypass the normal CLI error path and can leak stack traces, while only a couple of branches hand-roll console.error/process.exit(1). Please make the behaviour consistent with the rest of the CLI.

♻️ Suggested pattern

 import { Command } from 'commander';
+import { withErrorHandling } from '../../lib/utils/api-helper.js';
 ...
 const listCommand = new Command('list')
   .description('List all configuration values')
   .option('--profile <name>', 'profile name')
-  .action((options) => {
+  .action(withErrorHandling(async (options) => {
     const config = loadConfig();
     const profileName = options.profile || config['default-profile'];
     const profile = config.profiles[profileName];

     if (!profile) {
-      console.error(`Profile "${profileName}" not found`);
-      process.exit(1);
+      throw new Error(`Profile "${profileName}" not found`);
     }
 ...
-  });
+  }));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/config/index.ts` around lines 14 - 95, All command action
handlers (listCommand, getCommand, setCommand, unsetCommand and the
profilesCommand subcommands 'list', 'use', 'delete') must be wrapped with the
shared withErrorHandling helper so thrown errors follow the CLI's normal error
path; replace each .action((...) => { ... }) callback with
.action(withErrorHandling(async (...) => { ... })) (or the appropriate sync
form) so loadConfig, getConfigValue, setConfigValue, unsetConfigValue,
listProfiles, setDefaultProfile and deleteProfile errors are caught and reported
consistently.

src/commands/apikeys/index.ts-11-20 (1)

11-20: ⚠️ Potential issue | 🟠 Major

Fix the parseInt option parser to use base-10 explicitly.

Commander calls custom option parsers as (value, previous), so parseInt interprets the previous/default value as the radix. This means --limit 10 becomes parseInt('10', 20) which evaluates to 20 instead of 10. Use a dedicated base-10 parser instead.

Safer parser for `--limit` and `--offset`

-import { Command } from 'commander';
+import { Command, InvalidOptionArgumentError } from 'commander';
 ...
+function parseNonNegativeInt(value: string): number {
+  const parsed = Number.parseInt(value, 10);
+  if (!Number.isInteger(parsed) || parsed < 0) {
+    throw new InvalidOptionArgumentError('must be a non-negative integer');
+  }
+  return parsed;
+}
+
 const listCommand = new Command('list')
   .description('List all API keys')
-  .option('--limit <number>', 'maximum number of results', parseInt, 20)
-  .option('--offset <number>', 'offset for pagination', parseInt, 0)
+  .option('--limit <number>', 'maximum number of results', parseNonNegativeInt, 20)
+  .option('--offset <number>', 'offset for pagination', parseNonNegativeInt, 0)

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/apikeys/index.ts` around lines 11 - 20, The custom option
parsers for listCommand (--limit and --offset) use plain parseInt which
Commander will call as (value, previous) causing the previous/default to be
treated as the radix; update the parsers to use an explicit base-10 parser
instead (e.g., replace parseInt with a small parser that calls parseInt(value,
10) or Number.parseInt(value, 10)) for both the --limit and --offset options on
the listCommand so numeric values parse correctly.

src/commands/experiments/create.ts-16-17 (1)

16-17: ⚠️ Potential issue | 🟠 Major

--description and --hypothesis are silently ignored.

Both flags are exposed on the CLI, but the non-file code path never copies them into data, so the create request drops user input without warning. If they are not supported yet, it would be better to hide them than advertise dead flags.

Also applies to: 65-69

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/experiments/create.ts` around lines 16 - 17, The CLI options
--description and --hypothesis are parsed but never merged into the request
payload in the non-file code path, so user input gets dropped; update the create
command handler in src/commands/experiments/create.ts to copy opts.description
and opts.hypothesis into the data object (the same keys used when reading a
file) before calling the experiment creation routine (e.g., where data is
constructed/used), and apply the same fix for the other branch referenced around
the 65-69 area; alternatively, if these flags are not supported, remove/hide
their .option declarations to avoid advertising dead flags.

src/commands/experiments/create.ts-21-22 (1)

21-22: ⚠️ Potential issue | 🟠 Major

Don't resolve credentials before the offline modes short-circuit.

--dry-run and --as-curl are described as non-API paths, but getAPIClientFromOptions() runs first and can fail on missing profile/API key before either branch returns.

Move client creation below the early returns

   .action(withErrorHandling(async (options) => {
     const globalOptions = getGlobalOptions(createCommand);
-    const client = await getAPIClientFromOptions(globalOptions);

     let data: Partial<Experiment>;
@@
     if (options.asCurl) {
       // ...
       return;
     }

+    const client = await getAPIClientFromOptions(globalOptions);
     const experiment = await client.createExperiment(data);

Also applies to: 81-107

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/experiments/create.ts` around lines 21 - 22, The code calls
getAPIClientFromOptions(globalOptions) before checking the offline branches
(--dry-run and --as-curl), causing credential resolution errors; move the client
creation (the await getAPIClientFromOptions(...) call that uses the
globalOptions from getGlobalOptions(createCommand)) to after the
early-return/short-circuit logic that handles --dry-run and --as-curl so those
flows don't attempt to resolve credentials, and make the same change for the
other similar block referenced (the code around the 81-107 region that also
constructs the API client).

src/commands/experiments/list.test.ts-9-10 (1)

9-10: ⚠️ Potential issue | 🟠 Major

These tests never execute the command they claim to cover.

The suite builds an APIClient and drives client.listExperiments() / client.searchExperiments() directly, so regressions in src/commands/experiments/list.ts—such as --page offset calculation, CLI date parsing, or Commander option wiring—would still pass. Please invoke listCommand or experimentsCommand via parseAsync() here instead of asserting the client in isolation.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/experiments/list.test.ts` around lines 9 - 10, The test
currently exercises createAPIClient and calls
client.listExperiments()/client.searchExperiments() directly, so CLI wiring in
listCommand/experimentsCommand is not exercised; update the suite (the block
using describe.skipIf/isLiveMode and the client variable) to invoke the actual
command via its parseAsync API — call listCommand.parseAsync([...args]) or
experimentsCommand.parseAsync([...args]) with the appropriate CLI flags (e.g.,
--page, date options) instead of directly asserting on client methods so the
Commander option parsing and pagination logic are exercised; keep the existing
test setup for mocking API responses but replace direct
client.listExperiments()/searchExperiments() calls with parseAsync invocations.

src/commands/experiments/create.ts-91-101 (1)

91-101: ⚠️ Potential issue | 🟠 Major

The generated cURL can be wrong, unsafe, and invalid.

This branch reconstructs endpoint/auth outside getAPIClientFromOptions(), so profile-resolved values can diverge from the real request. It also prints the raw API key and wraps JSON.stringify(data) in single quotes, which breaks as soon as a field contains an apostrophe and leaks the credential into logs or shell history.

Safer output shape

-      console.log(`  -H 'Authorization: Api-Key ${apiKey}' \\`);
-      console.log(`  -d '${JSON.stringify(data)}'`);
+      console.log(`  -H 'Authorization: Api-Key $ABSMARTLY_API_KEY' \\`);
+      console.log("  --data-binary `@-` <<'EOF'");
+      console.log(JSON.stringify(data, null, 2));
+      console.log('EOF');

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/experiments/create.ts` around lines 91 - 101, The cURL branch
reconstructs endpoint/auth outside getAPIClientFromOptions(), prints the raw API
key, and uses JSON.stringify(data) wrapped in single quotes which breaks on
apostrophes; update the options.asCurl branch to get the real endpoint and
headers from getAPIClientFromOptions() (or the returned client) instead of
re-reading globals, do not log or interpolate the raw Api-Key (omit it from
output or show a masked placeholder), and emit a safe cURL body (e.g., use a
heredoc or properly escape/encode the JSON payload) so fields with apostrophes
do not break the command; reference options.asCurl, getAPIClientFromOptions(),
endpoint/apiKey/data and ensure the header output uses the client-provided auth
header rather than printing the secret.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bced53e7-89b7-42cc-a326-0eeacd3dff21

📥 Commits

Reviewing files that changed from the base of the PR and between 768663e and 2395bcb.

📒 Files selected for processing (172)

• .gitignore
• .npmignore
• .prettierrc
• ABSOLUTE_FINAL_SUMMARY.md
• BRANDED_TYPES_IMPLEMENTATION.md
• CODE_REVIEW_FINDINGS.md
• COMPREHENSIVE_REVIEW_COMPLETE.md
• CRITICAL_FIXES_NEEDED.md
• DEEP_REVIEW_SUMMARY.md
• DISTRIBUTION.md
• FINAL_ERROR_AUDIT.md
• FINAL_REVIEW_COMPLETE.md
• FINAL_SUMMARY.md
• FINAL_TEST_COVERAGE_AUDIT.md
• GETTING_STARTED.md
• LICENSE
• MIGRATION_GUIDE.md
• MISSION_COMPLETE.md
• OPENAPI_INTEGRATION.md
• PARALLEL_FIXES_SUMMARY.md
• PORTING_SUMMARY.md
• PORT_COMPLETE.md
• PROJECT_STATUS.md
• README.md
• REVIEW_COMPLETE.md
• REVIEW_ROUND_5_COMPLETE.md
• REVIEW_ROUND_6_COMPLETE.md
• SECURITY_FIXES_COMPLETE.md
• SECURITY_FIXES_REPORT.md
• TEST_COVERAGE_ANALYSIS.md
• TEST_COVERAGE_SUMMARY.md
• TEST_QUALITY_AUDIT.md
• TEST_REPORT.md
• TYPE_DESIGN_ANALYSIS.md
• TYPE_DESIGN_ANALYSIS_PART2.md
• TYPE_SAFETY_10_OUT_OF_10.md
• TYPE_SAFETY_IMPLEMENTATION_GUIDE.md
• TYPE_SYSTEM_ANALYSIS.md
• ULTIMATE_FINAL_STATUS.md
• ULTRA_DEEP_ERROR_AUDIT_FINAL.md
• docs/plans/2026-03-03-api-mocks-integration-design.md
• docs/plans/2026-03-03-api-mocks-integration-plan.md
• package.json
• src/commands/api/api.test.ts
• src/commands/api/index.ts
• src/commands/apikeys/apikeys.test.ts
• src/commands/apikeys/index.ts
• src/commands/apps/apps.test.ts
• src/commands/apps/index.ts
• src/commands/auth/auth.test.ts
• src/commands/auth/index.ts
• src/commands/completion/completion.test.ts
• src/commands/completion/index.ts
• src/commands/config/config.test.ts
• src/commands/config/index.ts
• src/commands/doctor/doctor.test.ts
• src/commands/doctor/index.ts
• src/commands/envs/envs.test.ts
• src/commands/envs/index.ts
• src/commands/experiments/activity.test.ts
• src/commands/experiments/activity.ts
• src/commands/experiments/archive.test.ts
• src/commands/experiments/archive.ts
• src/commands/experiments/create.test.ts
• src/commands/experiments/create.ts
• src/commands/experiments/development.test.ts
• src/commands/experiments/development.ts
• src/commands/experiments/full-on.test.ts
• src/commands/experiments/full-on.ts
• src/commands/experiments/generate-template.test.ts
• src/commands/experiments/generate-template.ts
• src/commands/experiments/get.test.ts
• src/commands/experiments/get.ts
• src/commands/experiments/index.ts
• src/commands/experiments/list.test.ts
• src/commands/experiments/list.ts
• src/commands/experiments/restart.test.ts
• src/commands/experiments/restart.ts
• src/commands/experiments/schedule.test.ts
• src/commands/experiments/schedule.ts
• src/commands/experiments/search.test.ts
• src/commands/experiments/search.ts
• src/commands/experiments/start.test.ts
• src/commands/experiments/start.ts
• src/commands/experiments/stop.test.ts
• src/commands/experiments/stop.ts
• src/commands/experiments/update.test.ts
• src/commands/experiments/update.ts
• src/commands/flags/flags.test.ts
• src/commands/flags/index.ts
• src/commands/generate/generate-types.test.ts
• src/commands/generate/generate.test.ts
• src/commands/generate/index.ts
• src/commands/goals/goals.test.ts
• src/commands/goals/index.ts
• src/commands/goaltags/goaltags.test.ts
• src/commands/goaltags/index.ts
• src/commands/metriccategories/index.ts
• src/commands/metriccategories/metriccategories.test.ts
• src/commands/metrics/index.ts
• src/commands/metrics/metrics.test.ts
• src/commands/metrictags/index.ts
• src/commands/metrictags/metrictags.test.ts
• src/commands/open/index.ts
• src/commands/open/open.test.ts
• src/commands/permissions/index.ts
• src/commands/permissions/permissions.test.ts
• src/commands/roles/index.ts
• src/commands/roles/roles.test.ts
• src/commands/segments/index.ts
• src/commands/segments/segments.test.ts
• src/commands/setup/index.ts
• src/commands/setup/setup.test.ts
• src/commands/tags/index.ts
• src/commands/tags/tags.test.ts
• src/commands/teams/index.ts
• src/commands/teams/teams.test.ts
• src/commands/units/index.ts
• src/commands/units/units.test.ts
• src/commands/users/index.ts
• src/commands/users/users.test.ts
• src/commands/version/index.ts
• src/commands/version/version.test.ts
• src/commands/webhooks/index.ts
• src/commands/webhooks/webhooks.test.ts
• src/index.ts
• src/lib/api/admin.test.ts
• src/lib/api/branded-types.test.ts
• src/lib/api/branded-types.ts
• src/lib/api/client-errors.test.ts
• src/lib/api/client.test.ts
• src/lib/api/client.ts
• src/lib/api/experiments.test.ts
• src/lib/api/list-options.test.ts
• src/lib/api/list-validation.test.ts
• src/lib/api/openapi-types.ts
• src/lib/api/resources.test.ts
• src/lib/api/tags.test.ts
• src/lib/api/types-compat.ts
• src/lib/api/types.ts
• src/lib/api/validation.test.ts
• src/lib/config/config-validation.test.ts
• src/lib/config/config.test.ts
• src/lib/config/config.ts
• src/lib/config/keyring.test.ts
• src/lib/config/keyring.ts
• src/lib/config/profile-merge.test.ts
• src/lib/output/formatter.test.ts
• src/lib/output/formatter.ts
• src/lib/template/generator.test.ts
• src/lib/template/generator.ts
• src/lib/template/parser.test.ts
• src/lib/template/parser.ts
• src/lib/utils/api-helper.test.ts
• src/lib/utils/api-helper.ts
• src/lib/utils/date-parser.test.ts
• src/lib/utils/date-parser.ts
• src/lib/utils/error-handler.test.ts
• src/lib/utils/error-handler.ts
• src/lib/utils/validators.test.ts
• src/lib/utils/validators.ts
• src/lib/utils/version.ts
• src/test/helpers/command-reset.ts
• src/test/helpers/live-helpers.ts
• src/test/helpers/test-config.ts
• src/test/live/run-live-tests.ts
• src/test/mocks/error-handlers.ts
• src/test/mocks/server.ts
• src/test/schema-validation.test.ts
• src/test/setup.ts
• tsconfig.json
• vitest.config.ts

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Check if index.js exists at repository root
ls -la index.js 2>&1 || echo "No index.js found at root"

Repository: absmartly/cli-ts

Length of output: 141

---

🏁 Script executed:

# Read the plan document around lines 49-50
head -n 60 docs/plans/2026-03-03-api-mocks-integration-plan.md | tail -n 25

Repository: absmartly/cli-ts

Length of output: 740

---

🏁 Script executed:

# Check the current package.json
cat package.json

Repository: absmartly/cli-ts

Length of output: 2133

---

Remove the guidance to keep main: "index.js" or ensure the file is created.

The plan proposes source-only TypeScript exports (all .ts files in src/), yet suggests keeping main pointing to a non-existent index.js. This breaks consumers using tools or Node.js versions that consult main before or instead of exports. Either delete the main field, build and publish an index.js file, or update main to point to a TypeScript source entry.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@docs/plans/2026-03-03-api-mocks-integration-plan.md` around lines 49 - 50,
The guidance to keep "main": "index.js" while publishing source-only TypeScript
is incorrect; update the plan to either remove the "main" field, ensure a built
index.js is produced and published, or change "main" to point to a TypeScript
entry so consumers won’t break; also confirm the "./src/generated/schema" export
and the consumer file (cli-ts's openapi-types.ts) will resolve correctly after
you choose one of these options and update the package.json/export guidance in
the document accordingly.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🌐 Web query:

Does js-yamlv4 still exposeSAFE_SCHEMA, or is yaml.load()already safe by default? What is the recommended secure replacement for olderSAFE_SCHEMA guidance?

💡 Result:

• SAFE_SCHEMA: No—js-yaml v4 does not export SAFE_SCHEMA (nor the old DEFAULT_SAFE_SCHEMA / DEFAULT_FULL_SCHEMA). The migration guide explicitly says those were removed and to use DEFAULT_SCHEMA instead. [1]
• Is yaml.load() safe by default in v4? Yes—in v4 the old unsafe behavior was removed from the default loader; load() defaults to DEFAULT_SCHEMA and “by default, does not support regexps, functions and undefined” (those JS-specific tags were moved out). [2]
• Recommended secure replacement for old “use SAFE_SCHEMA / safeLoad()” guidance:
  • Use yaml.load(str) (equivalent to the old safeLoad) or be explicit: yaml.load(str, { schema: yaml.DEFAULT_SCHEMA }). [1][2]
  • If you want an even more restricted schema, use FAILSAFE_SCHEMA (strings/arrays/objects only) or JSON_SCHEMA depending on your needs. [2]
  • Only if you intentionally need the old unsafe JS tags (e.g. !!js/function), extend the schema with js-yaml-js-types (opt-in). [1]

Sources:
[1] js-yaml v3→v4 migration guide (schema + safe* changes)
[2] js-yaml README (default load() behavior + available schemas)
[3] js-yaml v4.1.0 index.js exports (no SAFE_SCHEMA, exports DEFAULT_SCHEMA, etc.)
[4] js-yaml v4 changelog (safe defaults / removals)

---

🏁 Script executed:

find . -name "package.json" -o -name "package-lock.json" -o -name "yarn.lock" -o -name "pnpm-lock.yaml" | head -5

Repository: absmartly/cli-ts

Length of output: 74

---

🏁 Script executed:

cat ./package.json

Repository: absmartly/cli-ts

Length of output: 2133

---

🏁 Script executed:

find . -name "SECURITY_FIXES_REPORT.md" -type f

Repository: absmartly/cli-ts

Length of output: 86

---

🏁 Script executed:

cat -n ./SECURITY_FIXES_REPORT.md | head -50

Repository: absmartly/cli-ts

Length of output: 1752

---

🏁 Script executed:

grep -n "yaml.load" src/lib/config/config.ts | head -5

Repository: absmartly/cli-ts

Length of output: 102

---

🏁 Script executed:

cat -n src/lib/config/config.ts | sed -n '105,115p'

Repository: absmartly/cli-ts

Length of output: 439

---

SECURITY_FIXES_REPORT.md contains an incorrect YAML remediation that does not match the actual codebase and would fail at runtime.

The report claims the fix applies yaml.load(content, { schema: yaml.SAFE_SCHEMA }), but:

• The actual code at line 109 of src/lib/config/config.ts is yaml.load(content) with no schema parameter.
• js-yaml v4.1.0 (the version in use) does not export SAFE_SCHEMA—that constant was removed in the v3→v4 migration.
• The claimed fix would fail at runtime with "SAFE_SCHEMA is undefined".

The actual code is already secure: in js-yaml v4, yaml.load() defaults to safe behaviour and is equivalent to the old safeLoad(), with dangerous JS-specific tags removed by default. The report should either be corrected to reflect the actual implementation, or updated to document that the existing code is already compliant without modification.

[coderabbitai]

Actionable comments posted: 9

Note

Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.

🟡 Minor comments (13)

src/commands/roles/roles.test.ts-43-48 (1)

43-48: ⚠️ Potential issue | 🟡 Minor

Assert the formatted list payload as well.

At the moment this still passes if the command calls printFormatted with the wrong data. Please verify that the fetched roles are what gets forwarded to the formatter.

Suggested tightening

   expect(mockClient.listRoles).toHaveBeenCalledWith(20, 0);
-  expect(printFormatted).toHaveBeenCalled();
+  expect(printFormatted).toHaveBeenCalledWith(
+    [{ id: 1, name: 'admin' }],
+    expect.anything(),
+  );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/roles/roles.test.ts` around lines 43 - 48, The test currently
only asserts that listRoles was called and printFormatted was invoked; update it
to also assert that printFormatted received the actual roles returned by
mockClient.listRoles. After calling rolesCommand.parseAsync([...]), capture or
define the mock return value used by mockClient.listRoles (the roles array) and
add an assertion like expect(printFormatted).toHaveBeenCalledWith(theRoles) (or
with an appropriate matcher) so the formatter receives the exact payload fetched
by listRoles; reference rolesCommand.parseAsync, mockClient.listRoles, and
printFormatted to locate and update the test.

src/commands/metriccategories/metriccategories.test.ts-50-55 (1)

50-55: ⚠️ Potential issue | 🟡 Minor

Update test assertion to use branded TagId type.

The getMetricCategory method expects a TagId branded type, but the test assertion passes a plain number. Change line 53 from:

expect(mockClient.getMetricCategory).toHaveBeenCalledWith(1);

to:

expect(mockClient.getMetricCategory).toHaveBeenCalledWith(TagId(1));

This requires importing TagId at the top of the test file. While the test passes at runtime since branded types are numbers, the assertion should match the method's type contract for clarity and type safety.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/metriccategories/metriccategories.test.ts` around lines 50 - 55,
The test should assert the branded TagId type rather than a raw number: update
the expectation in the 'should get metric category by id' test to call
mockClient.getMetricCategory with TagId(1) instead of 1, and add an import for
TagId at the top of the test file; locate the test around
metricCategoriesCommand.parseAsync([...]) and modify the
expect(mockClient.getMetricCategory).toHaveBeenCalledWith(...) to use TagId(1)
while leaving the expect(printFormatted) assertion unchanged.

src/commands/webhooks/webhooks.test.ts-77-85 (1)

77-85: ⚠️ Potential issue | 🟡 Minor

Assert that the invalid update exits before calling the API.

This test only checks stderr, so it would still pass if the command logged an error after sending an empty updateWebhook request. Please also assert that updateWebhook was not invoked on this path.

Suggested test tightening

   } catch (error) {
     if (!(error as Error).message.startsWith('process.exit')) throw error;
+    expect(mockClient.updateWebhook).not.toHaveBeenCalled();
     const errorOutput = consoleErrorSpy.mock.calls.flat().join(' ');
     expect(errorOutput).toContain('update field');
   }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/webhooks/webhooks.test.ts` around lines 77 - 85, The test
currently only asserts stderr output for the empty-update case; also assert that
the API call was never made by verifying the mocked updateWebhook method was not
invoked. After catching the process.exit error in the it('should reject update
with no fields'...) block (after building errorOutput via
consoleErrorSpy.mock.calls), add an assertion that the mock for updateWebhook
(the mocked API client's updateWebhook function used by webhooksCommand) has
zero calls (e.g., expect(mockedUpdateWebhook).not.toHaveBeenCalled()), ensuring
the command exits before attempting any API request.

src/lib/api/tags.test.ts-4-4 (1)

4-4: ⚠️ Potential issue | 🟡 Minor

Unused import: isLiveMode is imported but never used.

The isLiveMode constant is imported from test-config.js but is not referenced anywhere in this test file. Consider removing it to keep imports clean.

🧹 Proposed fix

-import { isLiveMode, TEST_BASE_URL, TEST_API_KEY } from '../../test/helpers/test-config.js';
+import { TEST_BASE_URL, TEST_API_KEY } from '../../test/helpers/test-config.js';

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/api/tags.test.ts` at line 4, The import line currently brings in
isLiveMode but that symbol is unused; remove isLiveMode from the imported
specifier (keep TEST_BASE_URL and TEST_API_KEY) in the import statement in
tags.test.ts and run the project's linter/tests to ensure no other references
exist; if isLiveMode was intended for a test branch, either add the missing
usage in the relevant test or remove the import as above.

src/commands/experiments/search.ts-7-12 (1)

7-12: ⚠️ Potential issue | 🟡 Minor

Validate --limit before calling the API.

This parser currently accepts partial and non-positive values, so --limit 10foo, --limit 0, and --limit -1 all get through to searchExperiments. Please fail fast here with a strict positive-integer check instead of relying on downstream API errors.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/experiments/search.ts` around lines 7 - 12, Validate the parsed
options.limit before calling client.searchExperiments: ensure the value parsed
by the option handler is a strict positive integer (reject non-numeric suffixes
like "10foo", zero, and negatives). In the action handler (the async function
passed to .action / withErrorHandling that calls getGlobalOptions,
getAPIClientFromOptions and then client.searchExperiments), add a guard that
checks Number.isInteger(options.limit) and options.limit > 0 and throw or return
a user-facing error if it fails so the API is never called with an invalid
limit; keep the rest of the flow (getAPIClientFromOptions,
client.searchExperiments) unchanged.

src/lib/api/client.ts-217-225 (1)

217-225: ⚠️ Potential issue | 🟡 Minor

Preserve zero-valued timestamp bounds.

Lines 217, 220, and 223 use truthy checks, so 0 is silently dropped even though the Unix epoch is a valid boundary. That makes the created_*, started_*, and stopped_* filters impossible to send when one side of the range is zero.

Suggested fix

-    if (options.created_after || options.created_before) {
+    if (options.created_after !== undefined || options.created_before !== undefined) {
       params.created_at = `${options.created_after ?? 0},${options.created_before ?? 0}`;
     }
-    if (options.started_after || options.started_before) {
+    if (options.started_after !== undefined || options.started_before !== undefined) {
       params.started_at = `${options.started_after ?? 0},${options.started_before ?? 0}`;
     }
-    if (options.stopped_after || options.stopped_before) {
+    if (options.stopped_after !== undefined || options.stopped_before !== undefined) {
       params.stopped_at = `${options.stopped_after ?? 0},${options.stopped_before ?? 0}`;
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/api/client.ts` around lines 217 - 225, The truthy checks for
options.created_after/created_before, options.started_after/started_before, and
options.stopped_after/stopped_before drop zero values; change those conditionals
to check for undefined (or null) instead so a 0 is preserved — e.g., replace if
(options.created_after || options.created_before) with if (options.created_after
!== undefined || options.created_before !== undefined) and do the same for the
started and stopped blocks before assigning params.created_at /
params.started_at / params.stopped_at using the existing `${... ?? 0},${... ??
0}` expression.

src/test/helpers/live-helpers.ts-31-33 (1)

31-33: ⚠️ Potential issue | 🟡 Minor

Make live experiment names collision-resistant.

Line 33 only uses Date.now(). Two helpers invoked within the same millisecond can generate the same experiment name, which makes live runs flaky under parallelism or fast retries.

Suggested fix

-    name: `vitest_live_${Date.now()}${nameSuffix}`,
+    name: `vitest_live_${Date.now()}_${Math.random().toString(36).slice(2, 8)}${nameSuffix}`,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/helpers/live-helpers.ts` around lines 31 - 33, The experiment name
generation in buildExperimentData currently uses only Date.now() (name:
`vitest_live_${Date.now()}${nameSuffix}`), which can collide if called within
the same millisecond; update buildExperimentData to include an additional
high-entropy component (e.g., crypto.randomUUID() or a short
crypto.randomBytes/Math.random()-based hex string or process.pid+increment)
appended to the name to make it collision-resistant while preserving the
existing nameSuffix behavior.

src/lib/utils/validators.ts-20-48 (1)

20-48: ⚠️ Potential issue | 🟡 Minor

Enforce JavaScript's safe integer limit for ID parsing.

Lines 33 and the related validatePositiveInteger() function in src/lib/api/branded-types.ts check only Number.isInteger(), allowing IDs like 9007199254740992 to pass even though JavaScript cannot represent nearby values precisely. This risks silent ID corruption. Add Number.isSafeInteger() checks to both parseIdGeneric() and validatePositiveInteger().

Suggested fix for parseIdGeneric

   if (!Number.isInteger(id)) {
     throw new Error(`Invalid ${typeName}: "${value}" must be an integer (got ${id})`);
   }
+
+  if (!Number.isSafeInteger(id)) {
+    throw new Error(`Invalid ${typeName}: "${value}" exceeds JavaScript's safe integer range`);
+  }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/utils/validators.ts` around lines 20 - 48, parseIdGeneric currently
only checks Number.isInteger which allows values beyond JavaScript's safe
integer range; update parseIdGeneric to also call Number.isSafeInteger(id) and
throw a clear Error when it fails (e.g., "must be a safe integer within
JavaScript's Number.MAX_SAFE_INTEGER") and likewise update
validatePositiveInteger in src/lib/api/branded-types.ts to include
Number.isSafeInteger(id) alongside the existing integer/positive checks so
oversized IDs like 9007199254740992 are rejected; ensure the error messages
reference the safe-integer requirement for consistency with parseIdGeneric.

src/lib/api/client.ts-137-147 (1)

137-147: ⚠️ Potential issue | 🟡 Minor

Add ECONNABORTED to the timeout error condition.

Axios reports request timeouts as ECONNABORTED by default. Line 142 only checks ETIMEDOUT, so timeout errors are skipped and fall through to the generic error message.

Suggested fix

-        } else if (error.code === 'ETIMEDOUT') {
+        } else if (error.code === 'ECONNABORTED' || error.code === 'ETIMEDOUT') {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/api/client.ts` around lines 137 - 147, The timeout branch that sets
apiError.message for request timeouts currently only checks error.code ===
'ETIMEDOUT' in the error handling block inside src/lib/api/client.ts; update
that conditional (the clause that assigns the "Request timeout" message for the
method/endpoint) to also treat 'ECONNABORTED' as a timeout so Axios timeouts are
handled the same way as 'ETIMEDOUT' (i.e., include 'ECONNABORTED' in the OR
check or use a set/array membership check when comparing error.code).

src/commands/goals/goals.test.ts-49-53 (1)

49-53: ⚠️ Potential issue | 🟡 Minor

Strengthen assertion for custom limit test.

The test verifies listGoals was called but doesn't assert that the custom limit value was actually passed to the API method. This weakens the test's ability to catch regressions.

💚 Proposed fix to verify limit is passed correctly

   it('should list goals with custom limit', async () => {
     await goalsCommand.parseAsync(['node', 'test', 'list', '--limit', '5']);

-    expect(mockClient.listGoals).toHaveBeenCalled();
+    expect(mockClient.listGoals).toHaveBeenCalledWith(5, 0);
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/goals/goals.test.ts` around lines 49 - 53, The test "should list
goals with custom limit" only checks that mockClient.listGoals was called but
not that the custom limit was forwarded; update the test for the
goalsCommand.parseAsync(['node','test','list','--limit','5']) case to assert
that mockClient.listGoals was called with an argument containing the limit 5
(e.g., use toHaveBeenCalledWith or
toHaveBeenCalledWith(expect.objectContaining({ limit: 5 })) targeting
mockClient.listGoals) so the test verifies the CLI passes the custom limit
through.

src/commands/experiments/create.ts-91-107 (1)

91-107: ⚠️ Potential issue | 🟡 Minor

Security: API key is printed in plaintext in curl output.

The --as-curl flag outputs the API key directly in the curl command, which could expose it in terminal history, logs, or screenshots. Consider masking or warning the user.

🛡️ Suggested improvement: Add warning or mask the key

     if (options.asCurl) {
       const endpoint = globalOptions.endpoint || process.env.ABSMARTLY_API_ENDPOINT || 'https://demo-2.absmartly.com/v1';
       const apiKey = globalOptions.apiKey || process.env.ABSMARTLY_API_KEY || '';
 
+      console.log(chalk.yellow('⚠️  Warning: This command includes your API key. Avoid sharing or logging this output.'));
       console.log(chalk.blue('🔧 cURL Command:'));
       console.log('');
       console.log(`curl -X POST '${endpoint}/experiments' \\`);
       console.log(`  -H 'Authorization: Api-Key ${apiKey}' \\`);

Alternatively, consider using $ABSMARTLY_API_KEY placeholder and instruct the user to set the environment variable.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/experiments/create.ts` around lines 91 - 107, The curl output
currently prints the API key in plaintext (see usage of options.asCurl and
variables endpoint/apiKey/data); change this to avoid exposing secrets by
replacing the literal Api-Key value with a safe placeholder or masked value and
add a one-line warning. Specifically, when options.asCurl is true, construct the
curl command so the header uses either "Api-Key $ABSMARTLY_API_KEY" (and print a
note telling the user to export that env var) or show a masked value like
"Api-Key *****" plus a warning message that the real key must be supplied via
the ABSMARTLY_API_KEY environment variable; update any logs that reference
apiKey to avoid printing the secret. Ensure the change touches the block that
builds the curl string and the printed tip (refer to options.asCurl, endpoint,
apiKey, data).

src/test/helpers/test-config.ts-3-9 (1)

3-9: ⚠️ Potential issue | 🟡 Minor

Non-null assertions may cause unclear runtime errors if environment variables are missing.

When USE_LIVE_API=1 is set but LIVE_API_URL or LIVE_API_KEY are missing, the non-null assertions (!) will pass but the values will be undefined, potentially causing confusing errors later in test execution.

🛡️ Suggested validation

 export const isLiveMode = process.env.USE_LIVE_API === '1';
 
+if (isLiveMode) {
+  if (!process.env.LIVE_API_URL) {
+    throw new Error('LIVE_API_URL must be set when USE_LIVE_API=1');
+  }
+  if (!process.env.LIVE_API_KEY) {
+    throw new Error('LIVE_API_KEY must be set when USE_LIVE_API=1');
+  }
+}
+
 export const TEST_BASE_URL = isLiveMode
   ? process.env.LIVE_API_URL!
   : 'https://api.absmartly.com/v1';
 
 export const TEST_API_KEY = isLiveMode
   ? process.env.LIVE_API_KEY!
   : 'test-api-key';

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/helpers/test-config.ts` around lines 3 - 9, When isLiveMode is true,
TEST_BASE_URL and TEST_API_KEY currently use non-null assertions which can lead
to undefined values and unclear failures; instead add explicit runtime
validation inside the module: check process.env.LIVE_API_URL and
process.env.LIVE_API_KEY when isLiveMode is true and throw a clear, descriptive
Error if either is missing (referencing TEST_BASE_URL, TEST_API_KEY and
isLiveMode to locate the code), otherwise continue to use the non-live defaults;
ensure the error message names the missing variable(s) so test failures point
immediately to the root cause.

src/commands/flags/flags.test.ts-59-64 (1)

59-64: ⚠️ Potential issue | 🟡 Minor

Assert the --limit value if this test is meant to cover it.

The current expectation only rechecks type: 'feature', so this test still passes if the command ignores --limit completely.

Proposed fix

     expect(mockClient.listExperiments).toHaveBeenCalledWith(
-      expect.objectContaining({ type: 'feature' })
+      expect.objectContaining({ type: 'feature', limit: 5 })
     );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/flags/flags.test.ts` around lines 59 - 64, The test calls
flagsCommand.parseAsync(['node','test','list','--limit','5']) but only asserts
mockClient.listExperiments was called with { type: 'feature' }; update the
assertion to also check the parsed limit (e.g. expect.objectContaining({ type:
'feature', limit: 5 }) or an explicit check that the second/appropriate arg
includes limit: 5) so the test fails if the command ignores the --limit flag;
locate the assertion around mockClient.listExperiments and add the limit check
there.

🧹 Nitpick comments (12)

src/commands/permissions/permissions.test.ts (1)

12-14: Consider removing unused spy declarations or adding error-handling tests.

The consoleSpy, consoleErrorSpy, and processExitSpy variables are declared, initialised, and restored but never used in any assertions. If they're only for suppressing output during tests, they could be simplified to inline expressions without storing references. Alternatively, consider adding tests for error scenarios (e.g., API failures) that would utilise these spies for assertions.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/permissions/permissions.test.ts` around lines 12 - 14, The test
declares consoleSpy, consoleErrorSpy, and processExitSpy but never asserts
against them; either remove these unused declarations and the matching
setup/teardown (consoleSpy, consoleErrorSpy, processExitSpy) and replace any
suppression with inline vi.spyOn(...) calls, or add explicit error-handling
tests that exercise failure paths (mock API failures) and assert the spies are
called (e.g., expect(consoleErrorSpy).toHaveBeenCalledWith(...) and
expect(processExitSpy).toHaveBeenCalled()) so the spies are actually used;
update beforeEach/afterEach accordingly and keep references only if used in
assertions.

src/commands/generate/generate.test.ts (1)

69-83: Consider verifying pagination parameters for more robust testing.

The test confirms that pagination occurs (2 calls) and results are aggregated, but it doesn't verify that listExperiments was called with the correct offset/limit parameters. This means the test would pass even if the implementation called listExperiments twice with the same offset.

♻️ Suggested enhancement

     expect(mockClient.listExperiments).toHaveBeenCalledTimes(2);
+    expect(mockClient.listExperiments).toHaveBeenNthCalledWith(1, expect.objectContaining({ offset: 0, limit: 100 }));
+    expect(mockClient.listExperiments).toHaveBeenNthCalledWith(2, expect.objectContaining({ offset: 100, limit: 100 }));
     const output = consoleSpy.mock.calls.flat().join('\n');

Adjust the expected parameters to match the actual implementation's pagination API contract.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/generate/generate.test.ts` around lines 69 - 83, Update the test
to assert that listExperiments was invoked with the correct pagination
parameters rather than only counting calls: when calling
generateCommand.parseAsync([...]) verify mockClient.listExperiments was first
called with the initial params (e.g., limit/offset/token that your
implementation uses) and then called a second time with the expected next-page
params (e.g., offset incremented or nextPageToken passed). Use the existing
mockClient.listExperiments call history to inspect arguments
(mockClient.listExperiments.mock.calls[0] and [1]) and assert they match the
pagination contract used by your implementation so the test fails if the same
offset is reused.

src/commands/metrictags/metrictags.test.ts (3)

11-41: Unused spies: consoleErrorSpy and processExitSpy are set up but never asserted.

These spies are initialised in beforeEach and restored in afterEach, but no test case uses them for assertions. If error-handling tests are planned, consider adding them; otherwise, this is dead code.

🧹 Option: Remove unused spies if no error tests are planned

 describe('metric-tags command', () => {
   let consoleSpy: ReturnType<typeof vi.spyOn>;
-  let consoleErrorSpy: ReturnType<typeof vi.spyOn>;
-  let processExitSpy: ReturnType<typeof vi.spyOn>;

   const mockClient = {
     ...
   };

   beforeEach(() => {
     vi.clearAllMocks();
     resetCommand(metricTagsCommand);
     vi.mocked(getAPIClientFromOptions).mockResolvedValue(mockClient as any);
     vi.mocked(getGlobalOptions).mockReturnValue({ output: 'table' } as any);
     vi.mocked(printFormatted).mockImplementation(() => {});
     consoleSpy = vi.spyOn(console, 'log').mockImplementation(() => {});
-    consoleErrorSpy = vi.spyOn(console, 'error').mockImplementation(() => {});
-    processExitSpy = vi.spyOn(process, 'exit').mockImplementation((code?) => {
-      throw new Error(`process.exit: ${code}`);
-    });
   });

   afterEach(() => {
     consoleSpy.mockRestore();
-    consoleErrorSpy.mockRestore();
-    processExitSpy.mockRestore();
   });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/metrictags/metrictags.test.ts` around lines 11 - 41, The test
file initializes unused spies consoleErrorSpy and processExitSpy in beforeEach
and restores them in afterEach; either remove their declarations and the
vi.spyOn/restore calls (and related types) to eliminate dead code, or keep them
and add explicit error-path tests that trigger and assert their behavior (for
example, mock getAPIClientFromOptions or mockClient methods to reject and assert
console.error was called via consoleErrorSpy and that process.exit was
invoked/throws via processExitSpy); update the declarations near let
consoleErrorSpy/processExitSpy and the vi.spyOn(...) lines inside beforeEach
(and corresponding mockRestore in afterEach) accordingly to reflect the chosen
approach.

---

43-77: Consider adding negative test cases for error scenarios.

The current suite only covers happy-path scenarios. Adding tests for API failures, missing required arguments, and invalid IDs would improve robustness. For example:

• What happens when createMetricTag rejects?
• What happens when delete is called without an ID?

This is optional for this PR but worth tracking.

Would you like me to open an issue to track adding error-handling tests for the metric-tags command?

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/metrictags/metrictags.test.ts` around lines 43 - 77, Add
negative test cases for metricTagsCommand to cover API failures, missing
required arguments, and invalid IDs: write tests that call
metricTagsCommand.parseAsync simulating mockClient.createMetricTag rejecting
(assert consoleSpy/printFormatted logs the error and the command handles the
rejection without uncaught exceptions), simulate mockClient.deleteMetricTag
rejecting and assert error output, call parseAsync(['node','test','delete']) and
parseAsync(['node','test','get','invalid']) to assert the command prints usage
or validation errors rather than calling the client, and verify mockClient
methods (createMetricTag, deleteMetricTag, getMetricTag) are not called when
arguments are invalid. Ensure you reference metricTagsCommand.parseAsync,
mockClient.createMetricTag/deleteMetricTag/getMetricTag, and
consoleSpy/printFormatted in the new tests.

---

65-69: Inconsistent assertion pattern: update test lacks output verification.

The create and delete tests verify console output contains success indicators, but update only asserts the API call. For consistency and to confirm user feedback, add an output assertion.

♻️ Suggested fix: Add output assertion for update

  it('should update a metric tag', async () => {
    await metricTagsCommand.parseAsync(['node', 'test', 'update', '1', '--tag', 'new']);

    expect(mockClient.updateMetricTag).toHaveBeenCalledWith(1, { tag: 'new' });
+    const output = consoleSpy.mock.calls.flat().join(' ');
+    expect(output).toContain('updated');
  });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/metrictags/metrictags.test.ts` around lines 65 - 69, The update
test for metricTagsCommand only asserts the API call
(mockClient.updateMetricTag) but doesn't verify user-facing output; update the
test so after calling
metricTagsCommand.parseAsync(['node','test','update','1','--tag','new']) you
also assert the console/output contains a success message indicating the tag was
updated (mirror the pattern used in the create/delete tests), referencing
metricTagsCommand.parseAsync and mockClient.updateMetricTag to locate the test
and add an expectation that the CLI prints a success indicator (e.g., contains
"updated" or the same success string used by the other tests).

src/commands/metriccategories/metriccategories.test.ts (1)

43-88: Consider adding tests for error scenarios and edge cases.

The current test suite covers the happy paths well. For more robust coverage, consider adding tests for:

• API client errors (e.g., network failures, 404 responses)
• Invalid ID formats (non-numeric strings)
• Pagination options for the list command if supported
• Unarchive functionality if the command supports --unarchive or similar

This is optional and could be addressed in a follow-up.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/metriccategories/metriccategories.test.ts` around lines 43 - 88,
Add tests covering error and edge cases for metricCategoriesCommand to harden
behavior: add unit tests that mock failures from mockClient methods (e.g.,
mockClient.listMetricCategories, getMetricCategory, createMetricCategory,
updateMetricCategory, archiveMetricCategory) to assert errors are
surfaced/handled, test invalid ID inputs by calling
metricCategoriesCommand.parseAsync(['node','test','get','abc']) and expecting a
validation/error, add pagination/list variations if list supports page/limit
flags to assert correct args passed, and add an unarchive scenario (e.g.,
archive command with an unarchive flag) to verify archiveMetricCategory is
called with false; implement these as additional it() blocks similar to the
existing tests and use console/error spies to assert error messages where
applicable.

src/test/schema-validation.test.ts (1)

67-70: Parameterise the repeated list-contract cases.

Lines 67-70 and 82-199 repeat the same arrange/assert shape for each collection endpoint. Pulling those into a small case table would make this file much easier to extend when new resources land.

Possible refactor

+const paginatedCases = [
+  { name: 'goals', key: 'goals', path: '/goals', build: () => createMockGoals(3) },
+  { name: 'segments', key: 'segments', path: '/segments', build: () => createMockSegments(3) },
+  { name: 'teams', key: 'teams', path: '/teams', build: () => createMockTeams(3) },
+  // ...
+] as const;
+
+for (const { name, key, path, build } of paginatedCases) {
+  describe(name, () => {
+    it('list response matches OpenAPI spec', async () => {
+      await expectSchemaValid(path, 'get', paginatedResponse(key, build()));
+    });
+  });
+}

Also applies to: 82-199

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/schema-validation.test.ts` around lines 67 - 70, The tests duplicate
the same arrange/assert pattern across multiple collection endpoints (see the
experiments test using paginatedResponse, createMockExperiments and
expectSchemaValid), so refactor into a parameterised table-driven test: create
an array of cases (each with resource name, mock factory function like
createMockExperiments, and path/operation) and replace the repeated describe/it
blocks with a single loop or test.each that calls paginatedResponse and
expectSchemaValid for each case; update the file-level tests to reference the
case properties instead of duplicated code so adding new resources only requires
adding an entry to the cases array.

src/commands/users/users.test.ts (1)

16-29: Recreate mockClient per test to avoid leaked mock state.

mockClient is shared across the suite, and vi.clearAllMocks() only clears call history—it does not reset mock implementations. If a test overrides one of these methods with a different implementation (e.g. mockRejectedValue), that override persists into subsequent tests. Reinitialising mockClient in beforeEach ensures each test case starts with a clean set of mocks.

♻️ Suggested change

-  const mockClient = {
-    listUsers: vi.fn().mockResolvedValue([{ id: 1, email: 'test@test.com' }]),
-    getUser: vi.fn().mockResolvedValue({ id: 1, email: 'test@test.com' }),
-    createUser: vi.fn().mockResolvedValue({ id: 99 }),
-    updateUser: vi.fn().mockResolvedValue({}),
-    archiveUser: vi.fn().mockResolvedValue({}),
-  };
+  const createMockClient = () => ({
+    listUsers: vi.fn().mockResolvedValue([{ id: 1, email: 'test@test.com' }]),
+    getUser: vi.fn().mockResolvedValue({ id: 1, email: 'test@test.com' }),
+    createUser: vi.fn().mockResolvedValue({ id: 99 }),
+    updateUser: vi.fn().mockResolvedValue({}),
+    archiveUser: vi.fn().mockResolvedValue({}),
+  });
+  let mockClient: ReturnType<typeof createMockClient>;

   beforeEach(() => {
     vi.clearAllMocks();
+    mockClient = createMockClient();
     resetCommand(usersCommand);
     vi.mocked(getAPIClientFromOptions).mockResolvedValue(mockClient as any);

Alternatively, replace vi.clearAllMocks() with vi.resetAllMocks() to reset implementations alongside call history, though the factory pattern above is more explicit.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/users/users.test.ts` around lines 16 - 29, The suite-level
mockClient is shared across tests causing leaked mock implementations; update
the beforeEach to recreate mockClient (the object with listUsers, getUser,
createUser, updateUser, archiveUser) inside beforeEach before calling
vi.mocked(getAPIClientFromOptions).mockResolvedValue(...) so each test gets a
fresh set of jest/vi mocks, or alternatively replace vi.clearAllMocks() with
vi.resetAllMocks() to fully reset implementations; ensure references to
mockClient and the mocked getAPIClientFromOptions remain consistent after the
change.

src/test/mocks/error-handlers.ts (1)

1-3: Consider using the shared TEST_BASE_URL constant for consistency.

The base URL is hardcoded here, whereas other test files (e.g., tags.test.ts) use TEST_BASE_URL from test-config.ts. Since error handlers are only used in mock mode, this works correctly, but using the shared constant would improve consistency if the mock URL ever changes.

♻️ Optional refactor

 import { createErrorHandlers } from 'absmartly-api-mocks/handlers';
+import { TEST_BASE_URL } from '../helpers/test-config.js';

-export const errorHandlers = createErrorHandlers('https://api.absmartly.com/v1');
+export const errorHandlers = createErrorHandlers(TEST_BASE_URL);

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/mocks/error-handlers.ts` around lines 1 - 3, Replace the hardcoded
URL in errorHandlers by importing and using the shared TEST_BASE_URL constant:
add an import for TEST_BASE_URL from test-config (the same module used in other
tests) and pass TEST_BASE_URL to createErrorHandlers instead of
'https://api.absmartly.com/v1' so mocks remain consistent if the base URL
changes; update the export const errorHandlers =
createErrorHandlers(TEST_BASE_URL) and keep the createErrorHandlers import
unchanged.

src/commands/goals/index.ts (1)

48-62: Consider expanding update options if the API supports more fields.

The update command currently only supports --description. If the API's updateGoal method accepts additional fields (e.g., --name), consider adding them here for feature parity with other resource commands. This aligns with the create command which accepts both name and description.

💡 Potential enhancement if API supports name updates

 const updateCommand = new Command('update')
   .description('Update a goal')
   .argument('<id>', 'goal ID', parseGoalId)
+  .option('--name <name>', 'new name')
   .option('--description <text>', 'new description')
   .action(withErrorHandling(async (id: GoalId, options) => {
     const globalOptions = getGlobalOptions(updateCommand);
     const client = await getAPIClientFromOptions(globalOptions);

     const data: Record<string, string> = {};
+    if (options.name !== undefined) data.name = options.name;
     if (options.description !== undefined) data.description = options.description;

     requireAtLeastOneField(data, 'update field');
     await client.updateGoal(id, data);
     console.log(chalk.green(`✓ Goal ${id} updated`));
   }));

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/goals/index.ts` around lines 48 - 62, The update command only
maps --description to the payload; if the API's updateGoal supports additional
fields (e.g., name) add matching CLI options on the Command
(updateCommand.option('--name <text>', 'new name')), parse and include them into
the data object (e.g., if (options.name !== undefined) data.name =
options.name), keep requireAtLeastOneField(data, 'update field') and the call to
client.updateGoal(id, data), and update any typing for options if needed so
parseGoalId, updateCommand, requireAtLeastOneField, and client.updateGoal are
all used consistently.

src/test/setup.ts (1)

4-5: Consider adding a comment explaining why TLS verification is disabled.

Disabling NODE_TLS_REJECT_UNAUTHORIZED is security-sensitive. A brief comment would help future maintainers understand this is intentional for live testing scenarios (e.g., self-signed certificates in staging environments).

📝 Suggested documentation

 if (isLiveMode) {
+  // Disable TLS certificate verification for live testing against staging/dev environments
+  // that may use self-signed certificates. Do not use in production.
   process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0';
 } else {

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/setup.ts` around lines 4 - 5, Add a short inline comment next to the
isLiveMode block explaining why TLS verification is being disabled (e.g., to
allow self-signed certs in staging/live test environments) so future maintainers
understand the security tradeoff; update the code block containing isLiveMode
and the assignment to process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0' with that
explanatory comment and, if appropriate, mention any scope/constraints or TODO
to remove it in production.

src/commands/experiments/schedule.test.ts (1)

120-189: Consider using rejects matcher for cleaner async error testing.

The current try-catch pattern with throw new Error('Should have thrown') works but is verbose. Vitest's rejects matcher could simplify these tests.

♻️ Optional: Simplify error testing with rejects matcher

-    it('should reject invalid --action', async () => {
-      try {
-        await scheduleCommand.parseAsync([
-          'node', 'test', 'create', '42',
-          '--action', 'invalid',
-          '--at', '2030-04-01T10:00:00Z',
-        ]);
-        throw new Error('Should have thrown');
-      } catch (error) {
-        if ((error as Error).message.startsWith('process.exit')) {
-          const output = consoleErrorSpy.mock.calls.flat().join(' ');
-          expect(output).toContain('Invalid action');
-          expect(output).toContain('Valid actions');
-        } else {
-          throw error;
-        }
-      }
-    });
+    it('should reject invalid --action', async () => {
+      await expect(scheduleCommand.parseAsync([
+        'node', 'test', 'create', '42',
+        '--action', 'invalid',
+        '--at', '2030-04-01T10:00:00Z',
+      ])).rejects.toThrow('process.exit');
+
+      const output = consoleErrorSpy.mock.calls.flat().join(' ');
+      expect(output).toContain('Invalid action');
+      expect(output).toContain('Valid actions');
+    });

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/experiments/schedule.test.ts` around lines 120 - 189, Replace
the try/catch "Should have thrown" pattern in the three tests ('should reject
invalid --action', 'should reject invalid datetime', 'should reject past
datetime') by using Vitest's rejects matcher: call
expect(scheduleCommand.parseAsync(...)).rejects.toThrow() (or
.rejects.toThrow(/message/)) and then assert consoleErrorSpy.mock.calls contains
the expected error lines; update references to scheduleCommand.parseAsync and
consoleErrorSpy in each test accordingly to keep the same message assertions but
without manual try/catch.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/commands/experiments/schedule.ts`:
- Around line 30-48: The code currently accepts timezone-less datetimes via
options.at and then constructs a Date (variable date) which can change the
intended instant when normalized to UTC before calling
client.createScheduledAction; add an explicit pre-check on options.at to reject
strings that lack a timezone designator (no trailing "Z" or "+/-HH:MM" offset)
and throw a clear error (e.g., “Provide an ISO 8601 timestamp with Z or an
offset”), then only parse into new Date(options.at) after the check so
scheduling uses an unambiguous instant.

In `@src/index.ts`:
- Around line 40-49: Top-level help advertises global flags (--config,
--profile, --endpoint, --no-color) that are not applied consistently because
openCommand and doctorCommand bypass parsed globals and
setupCommand/handleFatalError emit colored output without checking the parsed
no-color flag; fix by wiring the parsed global options into early CLI bootstrap
so they are applied before command handlers run, update openCommand and
doctorCommand to read configuration/profile/endpoint from the shared parsed
options/context instead of reading defaults directly, and modify setupCommand
and handleFatalError to consult the same no-color flag (or global color config)
when deciding whether to emit colored output so the global --no-color behavior
is honored everywhere.
- Around line 81-84: The command tree uses async action handlers but calls
program.parse(), which doesn't await async handlers; replace the call to
program.parse() with program.parseAsync() so Commander will properly await async
subcommand handlers and integrate their errors into Commander’s error flow;
update any surrounding code that depends on the returned Promise (if any) to
await the parseAsync() call or handle its Promise accordingly.

In `@src/lib/api/client.ts`:
- Around line 67-79: The current axiosRetry setup in the client (the axiosRetry
call on this.client) treats PUT as idempotent and will auto-retry PUTs; change
the retryCondition to use an explicit allowlist of safe idempotent endpoints
instead of relying on method alone: inspect error.config.method and
error.config.url (or pathname) and only allow retries when the request
URL+method matches a configured set of known-safe routes (e.g., list only truly
idempotent endpoints and exclude action routes like startExperiment,
developmentExperiment, restartExperiment, fullOnExperiment), keeping the
existing network-error and 5xx response logic but gated by this allowlist so
action PUTs are not retried.
- Around line 719-739: The path validation in rawRequest decodes the incoming
path into decodedPath but only blocks '://', allowing protocol-relative URLs
like "//evil.com/path" which Axios treats as absolute and bypasses baseURL;
update the validation in the same block that inspects decodedPath (the checks
around decodedPath.includes('://'), decodedPath.startsWith('/'), and traversal
checks) to also reject protocol-relative paths (e.g., throw the same "Invalid
API path" error when decodedPath starts with "//" or matches a
leading-protocol-relative pattern) so that any path beginning with "//" is
treated as invalid and cannot be used for SSRF.

In `@src/lib/utils/validators.ts`:
- Around line 134-142: The validateJSON function currently includes a raw slice
of the input JSON in the thrown error, which can leak secrets; update the throw
in validateJSON to never echo raw input — remove the `Input:
${jsonString.substring...}` piece and instead include only non-sensitive
metadata (e.g., the input length and/or a deterministic hash like SHA256 or a
fixed "<redacted>" notice) while still preserving the original error.message;
modify the error construction in validateJSON (the thrown Error) accordingly.

In `@src/test/live/run-live-tests.ts`:
- Around line 176-187: The test bypasses the public API by calling
client.rawRequest and manually unwrapping new_experiment; change the test to
call client.restartExperiment(...) instead so the live suite exercises the real
client method (restartExperiment) and uses its return shape; update the test
inside the 'Restart experiment' runTest block to invoke restartExperiment with
the same payload (reason/state/data), assert on the returned value per the
restartExperiment contract, and assign experimentId from the returned object's
id (using ExperimentId) and log its state—this ensures the public API
(restartExperiment) stays exercised and validated by the live tests.

In `@src/test/schema-validation.test.ts`:
- Line 29: The test currently constructs specPath using a hard-coded
'node_modules' path via resolve; replace that with Node's module resolution by
using require.resolve to locate
'absmartly-api-mocks/openapi/openapi.bundle.yaml' so resolution honors package
managers and CWD. Update the assignment to set specPath using
require.resolve('absmartly-api-mocks/openapi/openapi.bundle.yaml') (remove the
hard-coded path usage and the reliance on resolve for that lookup) so the module
is found consistently across pnpm/Yarn/npm and monorepo layouts.

In `@tsconfig.json`:
- Around line 4-6: TS config uses "module": "ES2022" but keeps the legacy
resolver "moduleResolution": "node", which can mis-handle ESM conditional
exports; update the tsconfig by changing the "moduleResolution" option to either
"node16" or "nodenext" so TypeScript resolves ESM packages the same way Node
does (also ensure package.json has "type": "module" set), i.e., replace the
"moduleResolution": "node" entry with "moduleResolution": "node16" (or
"nodenext") to align TypeScript's resolver with Node ESM semantics.

---

Minor comments:
In `@src/commands/experiments/create.ts`:
- Around line 91-107: The curl output currently prints the API key in plaintext
(see usage of options.asCurl and variables endpoint/apiKey/data); change this to
avoid exposing secrets by replacing the literal Api-Key value with a safe
placeholder or masked value and add a one-line warning. Specifically, when
options.asCurl is true, construct the curl command so the header uses either
"Api-Key $ABSMARTLY_API_KEY" (and print a note telling the user to export that
env var) or show a masked value like "Api-Key *****" plus a warning message that
the real key must be supplied via the ABSMARTLY_API_KEY environment variable;
update any logs that reference apiKey to avoid printing the secret. Ensure the
change touches the block that builds the curl string and the printed tip (refer
to options.asCurl, endpoint, apiKey, data).

In `@src/commands/experiments/search.ts`:
- Around line 7-12: Validate the parsed options.limit before calling
client.searchExperiments: ensure the value parsed by the option handler is a
strict positive integer (reject non-numeric suffixes like "10foo", zero, and
negatives). In the action handler (the async function passed to .action /
withErrorHandling that calls getGlobalOptions, getAPIClientFromOptions and then
client.searchExperiments), add a guard that checks
Number.isInteger(options.limit) and options.limit > 0 and throw or return a
user-facing error if it fails so the API is never called with an invalid limit;
keep the rest of the flow (getAPIClientFromOptions, client.searchExperiments)
unchanged.

In `@src/commands/flags/flags.test.ts`:
- Around line 59-64: The test calls
flagsCommand.parseAsync(['node','test','list','--limit','5']) but only asserts
mockClient.listExperiments was called with { type: 'feature' }; update the
assertion to also check the parsed limit (e.g. expect.objectContaining({ type:
'feature', limit: 5 }) or an explicit check that the second/appropriate arg
includes limit: 5) so the test fails if the command ignores the --limit flag;
locate the assertion around mockClient.listExperiments and add the limit check
there.

In `@src/commands/goals/goals.test.ts`:
- Around line 49-53: The test "should list goals with custom limit" only checks
that mockClient.listGoals was called but not that the custom limit was
forwarded; update the test for the
goalsCommand.parseAsync(['node','test','list','--limit','5']) case to assert
that mockClient.listGoals was called with an argument containing the limit 5
(e.g., use toHaveBeenCalledWith or
toHaveBeenCalledWith(expect.objectContaining({ limit: 5 })) targeting
mockClient.listGoals) so the test verifies the CLI passes the custom limit
through.

In `@src/commands/metriccategories/metriccategories.test.ts`:
- Around line 50-55: The test should assert the branded TagId type rather than a
raw number: update the expectation in the 'should get metric category by id'
test to call mockClient.getMetricCategory with TagId(1) instead of 1, and add an
import for TagId at the top of the test file; locate the test around
metricCategoriesCommand.parseAsync([...]) and modify the
expect(mockClient.getMetricCategory).toHaveBeenCalledWith(...) to use TagId(1)
while leaving the expect(printFormatted) assertion unchanged.

In `@src/commands/roles/roles.test.ts`:
- Around line 43-48: The test currently only asserts that listRoles was called
and printFormatted was invoked; update it to also assert that printFormatted
received the actual roles returned by mockClient.listRoles. After calling
rolesCommand.parseAsync([...]), capture or define the mock return value used by
mockClient.listRoles (the roles array) and add an assertion like
expect(printFormatted).toHaveBeenCalledWith(theRoles) (or with an appropriate
matcher) so the formatter receives the exact payload fetched by listRoles;
reference rolesCommand.parseAsync, mockClient.listRoles, and printFormatted to
locate and update the test.

In `@src/commands/webhooks/webhooks.test.ts`:
- Around line 77-85: The test currently only asserts stderr output for the
empty-update case; also assert that the API call was never made by verifying the
mocked updateWebhook method was not invoked. After catching the process.exit
error in the it('should reject update with no fields'...) block (after building
errorOutput via consoleErrorSpy.mock.calls), add an assertion that the mock for
updateWebhook (the mocked API client's updateWebhook function used by
webhooksCommand) has zero calls (e.g.,
expect(mockedUpdateWebhook).not.toHaveBeenCalled()), ensuring the command exits
before attempting any API request.

In `@src/lib/api/client.ts`:
- Around line 217-225: The truthy checks for
options.created_after/created_before, options.started_after/started_before, and
options.stopped_after/stopped_before drop zero values; change those conditionals
to check for undefined (or null) instead so a 0 is preserved — e.g., replace if
(options.created_after || options.created_before) with if (options.created_after
!== undefined || options.created_before !== undefined) and do the same for the
started and stopped blocks before assigning params.created_at /
params.started_at / params.stopped_at using the existing `${... ?? 0},${... ??
0}` expression.
- Around line 137-147: The timeout branch that sets apiError.message for request
timeouts currently only checks error.code === 'ETIMEDOUT' in the error handling
block inside src/lib/api/client.ts; update that conditional (the clause that
assigns the "Request timeout" message for the method/endpoint) to also treat
'ECONNABORTED' as a timeout so Axios timeouts are handled the same way as
'ETIMEDOUT' (i.e., include 'ECONNABORTED' in the OR check or use a set/array
membership check when comparing error.code).

In `@src/lib/api/tags.test.ts`:
- Line 4: The import line currently brings in isLiveMode but that symbol is
unused; remove isLiveMode from the imported specifier (keep TEST_BASE_URL and
TEST_API_KEY) in the import statement in tags.test.ts and run the project's
linter/tests to ensure no other references exist; if isLiveMode was intended for
a test branch, either add the missing usage in the relevant test or remove the
import as above.

In `@src/lib/utils/validators.ts`:
- Around line 20-48: parseIdGeneric currently only checks Number.isInteger which
allows values beyond JavaScript's safe integer range; update parseIdGeneric to
also call Number.isSafeInteger(id) and throw a clear Error when it fails (e.g.,
"must be a safe integer within JavaScript's Number.MAX_SAFE_INTEGER") and
likewise update validatePositiveInteger in src/lib/api/branded-types.ts to
include Number.isSafeInteger(id) alongside the existing integer/positive checks
so oversized IDs like 9007199254740992 are rejected; ensure the error messages
reference the safe-integer requirement for consistency with parseIdGeneric.

In `@src/test/helpers/live-helpers.ts`:
- Around line 31-33: The experiment name generation in buildExperimentData
currently uses only Date.now() (name: `vitest_live_${Date.now()}${nameSuffix}`),
which can collide if called within the same millisecond; update
buildExperimentData to include an additional high-entropy component (e.g.,
crypto.randomUUID() or a short crypto.randomBytes/Math.random()-based hex string
or process.pid+increment) appended to the name to make it collision-resistant
while preserving the existing nameSuffix behavior.

In `@src/test/helpers/test-config.ts`:
- Around line 3-9: When isLiveMode is true, TEST_BASE_URL and TEST_API_KEY
currently use non-null assertions which can lead to undefined values and unclear
failures; instead add explicit runtime validation inside the module: check
process.env.LIVE_API_URL and process.env.LIVE_API_KEY when isLiveMode is true
and throw a clear, descriptive Error if either is missing (referencing
TEST_BASE_URL, TEST_API_KEY and isLiveMode to locate the code), otherwise
continue to use the non-live defaults; ensure the error message names the
missing variable(s) so test failures point immediately to the root cause.

---

Nitpick comments:
In `@src/commands/experiments/schedule.test.ts`:
- Around line 120-189: Replace the try/catch "Should have thrown" pattern in the
three tests ('should reject invalid --action', 'should reject invalid datetime',
'should reject past datetime') by using Vitest's rejects matcher: call
expect(scheduleCommand.parseAsync(...)).rejects.toThrow() (or
.rejects.toThrow(/message/)) and then assert consoleErrorSpy.mock.calls contains
the expected error lines; update references to scheduleCommand.parseAsync and
consoleErrorSpy in each test accordingly to keep the same message assertions but
without manual try/catch.

In `@src/commands/generate/generate.test.ts`:
- Around line 69-83: Update the test to assert that listExperiments was invoked
with the correct pagination parameters rather than only counting calls: when
calling generateCommand.parseAsync([...]) verify mockClient.listExperiments was
first called with the initial params (e.g., limit/offset/token that your
implementation uses) and then called a second time with the expected next-page
params (e.g., offset incremented or nextPageToken passed). Use the existing
mockClient.listExperiments call history to inspect arguments
(mockClient.listExperiments.mock.calls[0] and [1]) and assert they match the
pagination contract used by your implementation so the test fails if the same
offset is reused.

In `@src/commands/goals/index.ts`:
- Around line 48-62: The update command only maps --description to the payload;
if the API's updateGoal supports additional fields (e.g., name) add matching CLI
options on the Command (updateCommand.option('--name <text>', 'new name')),
parse and include them into the data object (e.g., if (options.name !==
undefined) data.name = options.name), keep requireAtLeastOneField(data, 'update
field') and the call to client.updateGoal(id, data), and update any typing for
options if needed so parseGoalId, updateCommand, requireAtLeastOneField, and
client.updateGoal are all used consistently.

In `@src/commands/metriccategories/metriccategories.test.ts`:
- Around line 43-88: Add tests covering error and edge cases for
metricCategoriesCommand to harden behavior: add unit tests that mock failures
from mockClient methods (e.g., mockClient.listMetricCategories,
getMetricCategory, createMetricCategory, updateMetricCategory,
archiveMetricCategory) to assert errors are surfaced/handled, test invalid ID
inputs by calling
metricCategoriesCommand.parseAsync(['node','test','get','abc']) and expecting a
validation/error, add pagination/list variations if list supports page/limit
flags to assert correct args passed, and add an unarchive scenario (e.g.,
archive command with an unarchive flag) to verify archiveMetricCategory is
called with false; implement these as additional it() blocks similar to the
existing tests and use console/error spies to assert error messages where
applicable.

In `@src/commands/metrictags/metrictags.test.ts`:
- Around line 11-41: The test file initializes unused spies consoleErrorSpy and
processExitSpy in beforeEach and restores them in afterEach; either remove their
declarations and the vi.spyOn/restore calls (and related types) to eliminate
dead code, or keep them and add explicit error-path tests that trigger and
assert their behavior (for example, mock getAPIClientFromOptions or mockClient
methods to reject and assert console.error was called via consoleErrorSpy and
that process.exit was invoked/throws via processExitSpy); update the
declarations near let consoleErrorSpy/processExitSpy and the vi.spyOn(...) lines
inside beforeEach (and corresponding mockRestore in afterEach) accordingly to
reflect the chosen approach.
- Around line 43-77: Add negative test cases for metricTagsCommand to cover API
failures, missing required arguments, and invalid IDs: write tests that call
metricTagsCommand.parseAsync simulating mockClient.createMetricTag rejecting
(assert consoleSpy/printFormatted logs the error and the command handles the
rejection without uncaught exceptions), simulate mockClient.deleteMetricTag
rejecting and assert error output, call parseAsync(['node','test','delete']) and
parseAsync(['node','test','get','invalid']) to assert the command prints usage
or validation errors rather than calling the client, and verify mockClient
methods (createMetricTag, deleteMetricTag, getMetricTag) are not called when
arguments are invalid. Ensure you reference metricTagsCommand.parseAsync,
mockClient.createMetricTag/deleteMetricTag/getMetricTag, and
consoleSpy/printFormatted in the new tests.
- Around line 65-69: The update test for metricTagsCommand only asserts the API
call (mockClient.updateMetricTag) but doesn't verify user-facing output; update
the test so after calling
metricTagsCommand.parseAsync(['node','test','update','1','--tag','new']) you
also assert the console/output contains a success message indicating the tag was
updated (mirror the pattern used in the create/delete tests), referencing
metricTagsCommand.parseAsync and mockClient.updateMetricTag to locate the test
and add an expectation that the CLI prints a success indicator (e.g., contains
"updated" or the same success string used by the other tests).

In `@src/commands/permissions/permissions.test.ts`:
- Around line 12-14: The test declares consoleSpy, consoleErrorSpy, and
processExitSpy but never asserts against them; either remove these unused
declarations and the matching setup/teardown (consoleSpy, consoleErrorSpy,
processExitSpy) and replace any suppression with inline vi.spyOn(...) calls, or
add explicit error-handling tests that exercise failure paths (mock API
failures) and assert the spies are called (e.g.,
expect(consoleErrorSpy).toHaveBeenCalledWith(...) and
expect(processExitSpy).toHaveBeenCalled()) so the spies are actually used;
update beforeEach/afterEach accordingly and keep references only if used in
assertions.

In `@src/commands/users/users.test.ts`:
- Around line 16-29: The suite-level mockClient is shared across tests causing
leaked mock implementations; update the beforeEach to recreate mockClient (the
object with listUsers, getUser, createUser, updateUser, archiveUser) inside
beforeEach before calling
vi.mocked(getAPIClientFromOptions).mockResolvedValue(...) so each test gets a
fresh set of jest/vi mocks, or alternatively replace vi.clearAllMocks() with
vi.resetAllMocks() to fully reset implementations; ensure references to
mockClient and the mocked getAPIClientFromOptions remain consistent after the
change.

In `@src/test/mocks/error-handlers.ts`:
- Around line 1-3: Replace the hardcoded URL in errorHandlers by importing and
using the shared TEST_BASE_URL constant: add an import for TEST_BASE_URL from
test-config (the same module used in other tests) and pass TEST_BASE_URL to
createErrorHandlers instead of 'https://api.absmartly.com/v1' so mocks remain
consistent if the base URL changes; update the export const errorHandlers =
createErrorHandlers(TEST_BASE_URL) and keep the createErrorHandlers import
unchanged.

In `@src/test/schema-validation.test.ts`:
- Around line 67-70: The tests duplicate the same arrange/assert pattern across
multiple collection endpoints (see the experiments test using paginatedResponse,
createMockExperiments and expectSchemaValid), so refactor into a parameterised
table-driven test: create an array of cases (each with resource name, mock
factory function like createMockExperiments, and path/operation) and replace the
repeated describe/it blocks with a single loop or test.each that calls
paginatedResponse and expectSchemaValid for each case; update the file-level
tests to reference the case properties instead of duplicated code so adding new
resources only requires adding an entry to the cases array.

In `@src/test/setup.ts`:
- Around line 4-5: Add a short inline comment next to the isLiveMode block
explaining why TLS verification is being disabled (e.g., to allow self-signed
certs in staging/live test environments) so future maintainers understand the
security tradeoff; update the code block containing isLiveMode and the
assignment to process.env.NODE_TLS_REJECT_UNAUTHORIZED = '0' with that
explanatory comment and, if appropriate, mention any scope/constraints or TODO
to remove it in production.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Don't advertise global overrides that some commands still ignore.

openCommand and doctorCommand read the default profile/config directly, and setupCommand plus handleFatalError emit coloured output without consulting parsed globals. That means --config, --profile, --endpoint, and --no-color are not actually global today, so the top-level help will promise behaviour the CLI does not deliver consistently.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/index.ts` around lines 40 - 49, Top-level help advertises global flags
(--config, --profile, --endpoint, --no-color) that are not applied consistently
because openCommand and doctorCommand bypass parsed globals and
setupCommand/handleFatalError emit colored output without checking the parsed
no-color flag; fix by wiring the parsed global options into early CLI bootstrap
so they are applied before command handlers run, update openCommand and
doctorCommand to read configuration/profile/endpoint from the shared parsed
options/context instead of reading defaults directly, and modify setupCommand
and handleFatalError to consult the same no-color flag (or global color config)
when deciding whether to emit colored output so the global --no-color behavior
is honored everywhere.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, let's find and read the file around the mentioned lines
fd "index.ts" --type f | head -5

Repository: absmartly/cli-ts

Length of output: 202

---

🏁 Script executed:

# Check the full context of src/index.ts around lines 81-84
wc -l src/index.ts

Repository: absmartly/cli-ts

Length of output: 75

---

🏁 Script executed:

# Read the file to understand the context
cat -n src/index.ts | sed -n '1,100p'

Repository: absmartly/cli-ts

Length of output: 4491

---

🏁 Script executed:

# Search for async handlers in the codebase
rg "async\s*\(" --type ts | head -20

Repository: absmartly/cli-ts

Length of output: 1898

---

🏁 Script executed:

# Look for Commander action handlers
rg "\.action\(" src/ -A 2 -B 1 --type ts

Repository: absmartly/cli-ts

Length of output: 35489

---

🌐 Web query:

Commander.js parseAsync async action handlers documentation

💡 Result:

Commander supports async .action() (and async hooks). If any action handler (or hook) returns a Promise / is declared async, you should use parseAsync() (and await it) so Commander waits for the async work to finish.

import { Command } from 'commander';

const program = new Command();

program
  .command('run')
  .action(async () => {
    await doWork();
  });

await program.parseAsync(process.argv);

Docs:

• README: “You may supply an async action handler, in which case you call .parseAsync() rather than .parse().” [1]
• API docs: parseAsync(argv?, parseOptions?) => Promise<this> and “Use parseAsync instead of parse if any of your action handlers are async.” [2]

Sources: [1] [2]

---

Use parseAsync() for this async command tree.

Multiple subcommands use async handlers throughout the command tree. Commander.js documentation explicitly requires .parseAsync() when any action handler is async. The current implementation with .parse() will not properly await async operations, potentially causing promise rejections to bypass Commander's error handling and fall through to the unhandledRejection handler instead.

Proposed fix

 process.on('unhandledRejection', (reason) => handleFatalError('unhandled promise rejection', reason));
 process.on('uncaughtException', (error) => handleFatalError('uncaught exception', error));
 
-program.parse();
+void program.parseAsync(process.argv).catch((error) => {
+  handleFatalError('cli bootstrap', error);
+});

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/index.ts` around lines 81 - 84, The command tree uses async action
handlers but calls program.parse(), which doesn't await async handlers; replace
the call to program.parse() with program.parseAsync() so Commander will properly
await async subcommand handlers and integrate their errors into Commander’s
error flow; update any surrounding code that depends on the returned Promise (if
any) to await the parseAsync() call or handle its Promise accordingly.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@README.md`:
- Line 32: Update the README to show platform-specific resolved config paths
instead of the Unix-only "~/.config/absmartly/config.yaml": add a short note
with examples for Unix (~/ .config/absmartly/config.yaml) and Windows
(%USERPROFILE%\.config\absmartly\config.yaml) and/or document or implement a CLI
command "config show-path" that prints the resolved path (using the existing
os.homedir() and path.join() logic) so users can discover their actual config
location on their OS.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 07a76d36-1e66-44b8-bc72-5c97d76ee12e

📥 Commits

Reviewing files that changed from the base of the PR and between f4acb0b and 00eb92f.

📒 Files selected for processing (1)

• README.md

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "Search for config path resolution in the codebase:"
rg -nP -C2 'config\.yaml|XDG|APPDATA|platformdirs|homedir|os\.homedir|keytar' .

Repository: absmartly/cli-ts

Length of output: 11257

---

🏁 Script executed:

rg -n 'getConfigDir|config.*path|show.*config|print.*config' --type ts --type md

Repository: absmartly/cli-ts

Length of output: 1235

---

🏁 Script executed:

rg -n 'config.*command|command.*config|show-config|describe' src/index.ts -A 5 -B 5

Repository: absmartly/cli-ts

Length of output: 648

---

🏁 Script executed:

find src -name "*config*" -type f | head -20

Repository: absmartly/cli-ts

Length of output: 222

---

🏁 Script executed:

cat -n src/commands/config/index.ts

Repository: absmartly/cli-ts

Length of output: 3816

---

🏁 Script executed:

cat -n src/lib/config/config.ts | head -50

Repository: absmartly/cli-ts

Length of output: 1544

---

Consider clarifying the config path for cross-platform consistency.

The implementation correctly uses os.homedir() and path.join() for cross-platform path resolution, but the documentation at lines 32 and 381 uses Unix-specific ~/.config/ syntax, which may be unclear for Windows users. Either include platform-specific path examples in the README (e.g. ~/.config/absmartly/config.yaml on Unix, %USERPROFILE%\.config\absmartly\config.yaml on Windows) or add a CLI command like config show-path to help users discover their system's resolved config location.

Also applies to: 381-381

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@README.md` at line 32, Update the README to show platform-specific resolved
config paths instead of the Unix-only "~/.config/absmartly/config.yaml": add a
short note with examples for Unix (~/ .config/absmartly/config.yaml) and Windows
(%USERPROFILE%\.config\absmartly\config.yaml) and/or document or implement a CLI
command "config show-path" that prints the resolved path (using the existing
os.homedir() and path.join() logic) so users can discover their actual config
location on their OS.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

src/test/schema-validation.test.ts (1)

30-32: Path resolution couples to internal package structure.

The switch to import.meta.resolve is an improvement over hardcoded node_modules paths. However, navigating up two directories from the validation entry assumes a specific internal layout of absmartly-api-mocks. Consider adding the OpenAPI spec to the package's exports field so it can be resolved directly:

const specPath = fileURLToPath(import.meta.resolve('absmartly-api-mocks/openapi/openapi.bundle.yaml'));

If modifying the package isn't feasible, the current approach is acceptable since both packages appear to be under the same organisation's control.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/test/schema-validation.test.ts` around lines 30 - 32, The current path
resolution walks up from validationEntry (using
fileURLToPath(import.meta.resolve(...)) and mocksDir) which couples to
absmartly-api-mocks internal layout; update specPath resolution to directly
resolve the package-exported spec via
import.meta.resolve('absmartly-api-mocks/openapi/openapi.bundle.yaml') so
specPath is obtained without traversing parent dirs (change the specPath
construction; ensure fileURLToPath(import.meta.resolve(...)) is used against the
export path), and if you cannot add the export to the package, keep the existing
fallback logic but add a clear comment referencing validationEntry and mocksDir
explaining the coupling.

src/commands/experiments/schedule.test.ts (1)

135-188: Assert that invalid input fails before command setup.

These cases only inspect stderr, so they still pass if the handler resolves global options or the API client before validating --action or --at. A couple of not.toHaveBeenCalled() assertions would lock in the intended short-circuit behaviour.

💡 Suggested test hardening

       } catch (error) {
         if ((error as Error).message.startsWith('process.exit')) {
           const output = consoleErrorSpy.mock.calls.flat().join(' ');
           expect(output).toContain('Invalid action');
           expect(output).toContain('Valid actions');
+          expect(getGlobalOptions).not.toHaveBeenCalled();
+          expect(getAPIClientFromOptions).not.toHaveBeenCalled();
         } else {
           throw error;
         }
       }
@@
       } catch (error) {
         if ((error as Error).message.startsWith('process.exit')) {
           const output = consoleErrorSpy.mock.calls.flat().join(' ');
           expect(output).toContain('Missing timezone');
+          expect(getGlobalOptions).not.toHaveBeenCalled();
+          expect(getAPIClientFromOptions).not.toHaveBeenCalled();
         } else {
           throw error;
         }
       }
@@
       } catch (error) {
         if ((error as Error).message.startsWith('process.exit')) {
           const output = consoleErrorSpy.mock.calls.flat().join(' ');
           expect(output).toContain('must be in the future');
+          expect(getGlobalOptions).not.toHaveBeenCalled();
+          expect(getAPIClientFromOptions).not.toHaveBeenCalled();
         } else {
           throw error;
         }
       }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/experiments/schedule.test.ts` around lines 135 - 188, Add
assertions that the command short-circuits before running any handler or API
call by spying the create handler and the API client and asserting they were not
invoked; specifically, before calling scheduleCommand.parseAsync(...) set up
spies for the command's create handler and the API client mock, then after
catching the process.exit-style error (after you collect
consoleErrorSpy.mock.calls.flat().join(' ')) add
expect(handlerSpy).not.toHaveBeenCalled() and
expect(apiClientMock.someMethod).not.toHaveBeenCalled() to each of the three
tests so invalid --action and malformed/old --at inputs never reach handler or
API code.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/commands/experiments/schedule.ts`:
- Around line 20-21: The global options and API client are being initialized
before validating local CLI flags, so failures in
getGlobalOptions(createScheduleCommand) or getAPIClientFromOptions(...) can mask
malformed local input like --action or --at; move the calls to
getGlobalOptions(createScheduleCommand) and
getAPIClientFromOptions(globalOptions) to after the local validation block that
checks the --action and --at flags so that local validation runs first and
returns the proper validation error before any configuration/API credential
loading occurs.

In `@src/commands/open/index.ts`:
- Around line 30-35: Replace the permissive parseInt-based ID handling in the
open command with the shared strict parser: remove the parseInt/isNaN/numericId
block that accepts things like "123abc" and instead call parseId(id) (from the
shared validator) to validate/parse the input; use the returned numeric id when
building webURL (webURL += `/${resource}/${id}`) and throw or let parseId's
error propagate on invalid input. Also add the import for parseId (import {
parseId } from '../../lib/utils/validators.js') at the top of the file so the
function is available.

In `@src/lib/api/client.ts`:
- Around line 67-88: The retryCondition currently checks
NON_IDEMPOTENT_PUT_PATHS against the raw error.config.url which can include
query strings or trailing slashes and miss matches; update the logic in the
axiosRetry configuration (retryCondition) to normalize the URL before matching
by extracting the pathname (strip query string and fragment), remove any
trailing slashes, and then perform the endsWith check for
NON_IDEMPOTENT_PUT_PATHS when method === 'PUT'; ensure the normalization handles
relative URLs (e.g., use a URL constructor with a base or fall back to string
operations) and keep references to NON_IDEMPOTENT_PUT_PATHS and
error.config.url/retryCondition so the change is easy to locate.

---

Nitpick comments:
In `@src/commands/experiments/schedule.test.ts`:
- Around line 135-188: Add assertions that the command short-circuits before
running any handler or API call by spying the create handler and the API client
and asserting they were not invoked; specifically, before calling
scheduleCommand.parseAsync(...) set up spies for the command's create handler
and the API client mock, then after catching the process.exit-style error (after
you collect consoleErrorSpy.mock.calls.flat().join(' ')) add
expect(handlerSpy).not.toHaveBeenCalled() and
expect(apiClientMock.someMethod).not.toHaveBeenCalled() to each of the three
tests so invalid --action and malformed/old --at inputs never reach handler or
API code.

In `@src/test/schema-validation.test.ts`:
- Around line 30-32: The current path resolution walks up from validationEntry
(using fileURLToPath(import.meta.resolve(...)) and mocksDir) which couples to
absmartly-api-mocks internal layout; update specPath resolution to directly
resolve the package-exported spec via
import.meta.resolve('absmartly-api-mocks/openapi/openapi.bundle.yaml') so
specPath is obtained without traversing parent dirs (change the specPath
construction; ensure fileURLToPath(import.meta.resolve(...)) is used against the
export path), and if you cannot add the export to the package, keep the existing
fallback logic but add a clear comment referencing validationEntry and mocksDir
explaining the coupling.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 434359e9-9a5b-439f-ba1f-de2d507e4e03

📥 Commits

Reviewing files that changed from the base of the PR and between 00eb92f and eb21056.

📒 Files selected for processing (12)

• src/commands/doctor/index.ts
• src/commands/experiments/schedule.test.ts
• src/commands/experiments/schedule.ts
• src/commands/open/index.ts
• src/index.ts
• src/lib/api/client.test.ts
• src/lib/api/client.ts
• src/lib/utils/validators.test.ts
• src/lib/utils/validators.ts
• src/test/live/run-live-tests.ts
• src/test/schema-validation.test.ts
• tsconfig.json

🚧 Files skipped from review as they are similar to previous changes (4)

• src/commands/doctor/index.ts
• src/test/live/run-live-tests.ts
• tsconfig.json
• src/index.ts

[coderabbitai]

⚠️ Potential issue | 🟠 Major

Validate local CLI input before loading configuration or API credentials.

getGlobalOptions() and getAPIClientFromOptions() run before the local --action and --at checks, so malformed input can be masked by a configuration/API-key failure instead of the real validation message. Move the setup until after the local validation block succeeds.

💡 Suggested fix

   .action(withErrorHandling(async (id: ExperimentId, options) => {
-    const globalOptions = getGlobalOptions(createScheduleCommand);
-    const client = await getAPIClientFromOptions(globalOptions);
-
     if (!VALID_ACTIONS.includes(options.action as any)) {
       throw new Error(
         `Invalid action: "${options.action}"\n` +
         `Valid actions: ${VALID_ACTIONS.join(', ')}`
       );
@@
     if (date.getTime() <= Date.now()) {
       throw new Error(
         `Scheduled time must be in the future.\n` +
         `Provided: ${options.at}`
       );
     }
+
+    const globalOptions = getGlobalOptions(createScheduleCommand);
+    const client = await getAPIClientFromOptions(globalOptions);
 
     const result = await client.createScheduledAction(
       id,

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/experiments/schedule.ts` around lines 20 - 21, The global
options and API client are being initialized before validating local CLI flags,
so failures in getGlobalOptions(createScheduleCommand) or
getAPIClientFromOptions(...) can mask malformed local input like --action or
--at; move the calls to getGlobalOptions(createScheduleCommand) and
getAPIClientFromOptions(globalOptions) to after the local validation block that
checks the --action and --at flags so that local validation runs first and
returns the proper validation error before any configuration/API credential
loading occurs.

[coderabbitai]

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify the current truncation behaviour of JavaScript's parseInt.
node - <<'NODE'
console.log(parseInt('123abc', 10))
console.log(parseInt('1e2', 10))
NODE

Repository: absmartly/cli-ts

Length of output: 65

---

🏁 Script executed:

# Check if parseId exists in validators.ts
cat -n src/lib/utils/validators.ts

Repository: absmartly/cli-ts

Length of output: 4807

---

🏁 Script executed:

# View the current state of the open command file around the flagged lines
cat -n src/commands/open/index.ts | head -50

Repository: absmartly/cli-ts

Length of output: 1945

---

🏁 Script executed:

# Search for parseId usage in the codebase to confirm it's used elsewhere
rg "parseId" --type ts

Repository: absmartly/cli-ts

Length of output: 4221

---

Use the shared strict ID validator to reject malformed inputs instead of truncating them.

Line 31 uses parseInt, so inputs such as 123abc and 1e2 resolve to 123 and 1 instead of being rejected, potentially opening the wrong resource. The codebase already provides a stricter shared parser at src/lib/utils/validators.ts that explicitly rejects any input with non-numeric characters. Replace the current parsing logic with parseId for consistency across the CLI.

Suggested change

       if (id) {
-        const numericId = parseInt(id, 10);
-        if (isNaN(numericId) || numericId <= 0) {
-          throw new Error(`Invalid resource ID: "${id}" - must be a positive integer`);
-        }
+        const numericId = parseId(id);
         webURL += `/${resource}/${numericId}`;

Also add the import at the top of the file:

import { parseId } from '../../lib/utils/validators.js';

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

	if (id) {
	const numericId = parseInt(id, 10);
	if (isNaN(numericId) || numericId <= 0) {
	throw new Error(`Invalid resource ID: "${id}" - must be a positive integer`);
	}
	webURL += `/${resource}/${numericId}`;
	if (id) {
	const numericId = parseId(id);
	webURL += `/${resource}/${numericId}`;

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/commands/open/index.ts` around lines 30 - 35, Replace the permissive
parseInt-based ID handling in the open command with the shared strict parser:
remove the parseInt/isNaN/numericId block that accepts things like "123abc" and
instead call parseId(id) (from the shared validator) to validate/parse the
input; use the returned numeric id when building webURL (webURL +=
`/${resource}/${id}`) and throw or let parseId's error propagate on invalid
input. Also add the import for parseId (import { parseId } from
'../../lib/utils/validators.js') at the top of the file so the function is
available.

[coderabbitai]

⚠️ Potential issue | 🔴 Critical

Normalise the URL before classifying non-idempotent PUTs.

Line 78 checks endsWith(...) against the raw URL string. rawRequest('/experiments/1/start?force=1', 'PUT') or /experiments/1/start/ will miss the suffix match, so a non-idempotent action can still be auto-retried after a dropped response.

🛠️ Suggested change

-        const url = error.config?.url ?? '';
+        const url = error.config?.url ?? '';
+        const pathname = url.split(/[?#]/, 1)[0].replace(/\/+$/, '');

-        if (method === 'PUT' && NON_IDEMPOTENT_PUT_PATHS.some((p) => url.endsWith(p))) {
+        if (
+          method === 'PUT' &&
+          NON_IDEMPOTENT_PUT_PATHS.some((p) => pathname.endsWith(p))
+        ) {
           return false;
         }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@src/lib/api/client.ts` around lines 67 - 88, The retryCondition currently
checks NON_IDEMPOTENT_PUT_PATHS against the raw error.config.url which can
include query strings or trailing slashes and miss matches; update the logic in
the axiosRetry configuration (retryCondition) to normalize the URL before
matching by extracting the pathname (strip query string and fragment), remove
any trailing slashes, and then perform the endsWith check for
NON_IDEMPOTENT_PUT_PATHS when method === 'PUT'; ensure the normalization handles
relative URLs (e.g., use a URL constructor with a base or fall back to string
operations) and keep references to NON_IDEMPOTENT_PUT_PATHS and
error.config.url/retryCondition so the change is easy to locate.