Improvements #35
Improvements #35
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughLarge refactor adding a centralized utilities layer (errors, retries, credential/profile management, output formatting), converting many commands to promise-based I/O with piped-aware output, adding new commands/docs/tests, and introducing packaging/CI tooling for building and releasing platform binaries. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant Utils
participant API
participant FS
User->>CLI: invoke command (with options/profile)
CLI->>Utils: getVerboseMode(), detect piping, validate input
CLI->>Utils: load credentials/profile
Utils->>FS: read credentials/profile file
Utils->>API: initializeApi / initializePbApi (returns client)
CLI->>Utils: createSpinner(enabled, text)
CLI->>CLI: validate/normalize inputs (cardKey, filenames)
CLI->>Utils: withRetry(() => API.call(...))
loop retries
Utils->>API: perform request
API-->>Utils: response / error
alt retriable error
Utils->>Utils: sleep then retry
end
end
CLI->>Utils: formatOutput(data, options)
alt write to file
Utils->>FS: writeFile (async)
FS-->>CLI: written
end
CLI-->>User: display result / exit (CliError printed via Utils)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds comprehensive test coverage for command functions and introduces substantial improvements to the codebase including better error handling, input validation, and utilities. The changes introduce Vitest as a testing framework and create tests for multiple command modules.
Key changes:
- Added Vitest configuration and test files for commands (deploy, upload, publish, fetch, accounts, balances, transactions, cards)
- Refactored template files to prefix unused function parameters with underscores
- Significant expansion of utils.ts with new utility functions for file operations, validation, terminal capabilities, and more
Reviewed Changes
Copilot reviewed 60 out of 76 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| vitest.config.ts | Adds Vitest configuration with test environment settings and coverage configuration |
| test/cmds/*.test.ts | New comprehensive test suites for command functions with mocking |
| templates/*/main.js | Prefixed unused function parameters with underscores to indicate intentional non-use |
| src/utils.ts | Massive expansion with new utilities for file operations, validation, terminal detection, and more |
| src/errors.ts | New error handling infrastructure with custom CliError class and exit codes |
| src/index.ts | Extensive refactoring with improved command definitions, help text, and error handling |
| src/function-calls.ts | Type improvements and better function signatures with validation |
| src/mock-*.ts | Code formatting changes (single to double quotes) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
templates/petro/main.js (1)
8-24: Remove underscore prefixes from function names to comply with documented API contract.The functions are part of a documented external API contract specified in
instructions.txt, which explicitly requires the functions to be namedbeforeTransaction,afterTransaction, andafterDeclineto be called by an external system. The underscore prefix renaming intemplates/petro/main.jsbreaks this contract and will cause runtime failures.Rename in
templates/petro/main.js:
- Line 8:
_beforeTransaction→beforeTransaction- Line 16:
_afterTransaction→afterTransaction- Line 21:
_afterDecline→afterDeclinesrc/function-calls.ts (1)
229-236: Remove the hard-coded transfer amount.Forcing
amount: '10'means the command always transfers ten units regardless of what the user asked for. That is a silent, dangerous correctness bug. Use the validatedoptions.amount.const transfer: TransferMultiple = { beneficiaryAccountId: options.beneficiaryAccountId, - amount: '10', // hardcoded for testing + amount: options.amount, myReference: options.myReference, theirReference: options.theirReference, };src/utils.ts (1)
1707-1734: Support relative credential file paths.Importing with
file://${credentialsFile}fails the moment someone passes a relative path—Node throwsERR_INVALID_URL. Resolve the path and feedpathToFileURLso both absolute and relative inputs keep working. Remember to addimport { pathToFileURL } from 'node:url';at the top.- try { - const file = await import(`file://${credentialsFile}`, { + try { + const resolvedPath = path.isAbsolute(credentialsFile) + ? credentialsFile + : path.resolve(credentialsFile); + const fileUrl = pathToFileURL(resolvedPath); + const file = await import(fileUrl.href, { with: { type: 'json' }, });src/mock-pb.ts (2)
74-87: Remove unnecessary type assertion in getAccountBalances.The mock data structure matches the expected
AccountBalanceResponsetype perfectly. The double type assertionas unknown as Promise<AccountBalanceResponse>at line 86 is unnecessary and bypasses TypeScript's type safety. Remove it:return Promise.resolve({ data: { accountId, currentBalance: 1000.0, availableBalance: 900.0, budgetBalance: 800.0, straightBalance: 700.0, cashBalance: 600.0, currency: 'ZAR', }, });
88-104: Remove the unnecessary double type assertion.The mock data structure
{ data: { accounts: [account] } }exactly matches the expectedAccountResponsetype from investec-pb-api. The Account interface has all required and optional fields properly defined, and your mock object provides all necessary properties. The double assertionas unknown as Promise<AccountResponse>at line 103 is unnecessary and should be removed.Change line 99-103 from:
return Promise.resolve({ data: { accounts: [account], }, }) as unknown as Promise<AccountResponse>;to:
return Promise.resolve({ data: { accounts: [account], }, });TypeScript will infer the correct type without the assertion, as evidenced by how downstream code in
src/function-calls.tsandsrc/cmds/accounts.tsaccessesresult.data.accountswithout type issues.
♻️ Duplicate comments (1)
templates/default/main.js (1)
2-12: Verify that the underscore prefix doesn't break external integrations.Similar to
templates/petro/main.js, these function names have been prefixed with underscores. If the Investec Programmable Banking platform expects specific function names (beforeTransaction,afterTransaction,afterDecline) when executing uploaded code, this change will break the integration.The verification script from the
templates/petro/main.jsreview will also check references to these function names.
🧹 Nitpick comments (13)
test/cmds/balances.test.ts (1)
23-24: Consider expanding test coverage for additional scenarios.The current tests cover basic success and error paths, but several important scenarios are not tested:
- Stdin input path (readStdin is hardcoded to return null)
- Piped output mode (isStdoutPiped is hardcoded to return false)
- JSON/YAML output options that are supported by the command
- Validation of actual console output content (currently only checks if console.log was called)
Consider adding tests like:
it('should read accountId from stdin when not provided', async () => { const { readStdin } = await import('../../src/utils.ts'); (readStdin as vi.Mock).mockResolvedValueOnce('acc-from-stdin'); // ... rest of test }); it('should output JSON when piped', async () => { const { isStdoutPiped } = await import('../../src/utils.ts'); (isStdoutPiped as vi.Mock).mockReturnValueOnce(true); // ... rest of test });Also applies to: 37-78
test/cmds/publish.test.ts (1)
31-31: Consider testing the cancellation path.Line 31 mocks
confirmDestructiveOperationto always returntrue, which means the user confirmation/cancellation path is never tested. According to src/cmds/publish.ts, the command should exit gracefully when the user declines.Add a test case:
it('should cancel when user declines confirmation', async () => { const { confirmDestructiveOperation } = await import('../../src/utils.ts'); (confirmDestructiveOperation as vi.Mock).mockResolvedValueOnce(false); await publishCommand(options); expect(mockApi.uploadPublishedCode).not.toHaveBeenCalled(); expect(console.log).toHaveBeenCalledWith('Publish cancelled.'); });Also applies to: 75-110
src/cmds/toggle.ts (1)
17-18: Potentially confusing option naming:spinnerto disable spinner.Line 17 uses
options.spinner === trueto determine if the spinner should be disabled. This naming is counterintuitive—a flag calledspinnertypically would enable the spinner, not disable it.Looking at the pattern across multiple commands, this appears to be intentional but could cause confusion for developers. The flag effectively means "suppress spinner" or "no-spinner" but is named just "spinner".
Consider renaming the option to be more explicit about its purpose:
noSpinnerordisableSpinner- makes the intent clear- Or document clearly in CommonOptions that
spinner: truedisables the spinnerAlternatively, invert the logic so
spinner: trueenables it (current behavior) andspinner: falseor undefined uses default behavior.src/cmds/pay.ts (1)
51-51:printTitleBox()is currently a no-op.The function is intentionally empty according to
src/index.ts(lines 77-79). While positioned for future use, it currently has no effect. Consider removing this call until the function is implemented, or add a comment explaining it's a placeholder for future UI enhancements.src/cmds/deploy.ts (1)
31-31: Consider showing title box before confirmation prompt.The
printTitleBox()is called at line 31 before validation and confirmation. In other commands like pay.ts (line 51), the title box is shown after prompts but before API initialization. Consider whether showing the title box before interactive confirmation prompts provides a better user experience.src/cmds/merchants.ts (2)
11-12: Unnecessary dynamic import ofisStdoutPiped.The function
isStdoutPipedis dynamically imported from../utils.js, but other utilities from the same module are already statically imported at line 2 (createSpinner,formatOutput,initializeApi). Dynamic imports add overhead and complexity without clear benefit here. This pattern is repeated in accounts.ts (line 16) and currencies.ts (line 11).Apply this diff to use a static import:
-import { createSpinner, formatOutput, initializeApi } from '../utils.js'; +import { createSpinner, formatOutput, initializeApi, isStdoutPiped } from '../utils.js'; import type { CommonOptions } from './types.js'; /** * Fetches and displays a list of merchants. * @param options - CLI options including API credentials * @throws {Error} When API credentials are invalid or API call fails */ export async function merchantsCommand(options: CommonOptions) { - const { isStdoutPiped } = await import('../utils.js'); const isPiped = isStdoutPiped();
28-29: Inconsistent empty array output format.Line 28 writes
'[]\n'to stdout for piped empty results, but line 42 in the non-empty case usesJSON.stringify(data, null, 2)viaformatOutputwhich produces formatted JSON. For consistency, empty arrays should also use the same formatting (either'[]'without newline or'[\n]\n'with proper formatting).src/cmds/accounts.ts (3)
16-17: Unnecessary dynamic import ofisStdoutPiped.Same issue as merchants.ts:
isStdoutPipedis dynamically imported but could be statically imported with other utilities from line 2.Apply this diff:
import { createSpinner, formatOutput, getVerboseMode, initializePbApi, + isStdoutPiped, withRetry, } from '../utils.js'; import type { CommonOptions } from './types.js'; /** * Fetch and display Investec accounts. * @param options CLI options */ export async function accountsCommand(options: CommonOptions) { - const { isStdoutPiped } = await import('../utils.js'); const isPiped = isStdoutPiped();
22-26: Redundant conditional spinner start.The
createSpinnerfunction (src/utils.ts lines 2335-2354) already returns a no-op spinner whenenabledisfalse. The conditional check at lines 24-26 to only callspinner.start()when not piped is redundant, as callingstart()on a disabled spinner is already a no-op.Simplify to:
const disableSpinner = options.spinner === true || isPiped; // Disable spinner when piped - const spinner = createSpinner(!disableSpinner, '💳 fetching accounts...'); - if (!isPiped) { - spinner.start(); - } + const spinner = createSpinner(!disableSpinner, '💳 fetching accounts...').start();
42-42: Inconsistent empty array output format.Same issue as merchants.ts line 28: writes
'[]\n'directly instead of using consistent JSON formatting.src/cmds/currencies.ts (2)
11-12: Unnecessary dynamic import ofisStdoutPiped.Same issue as merchants.ts and accounts.ts:
isStdoutPipedshould be statically imported.Apply this diff:
-import { createSpinner, formatOutput, initializeApi } from '../utils.js'; +import { createSpinner, formatOutput, initializeApi, isStdoutPiped } from '../utils.js'; import type { CommonOptions } from './types.js'; /** * Fetches and displays a list of supported currencies. * @param options - CLI options including API credentials * @throws {Error} When API credentials are invalid or API call fails */ export async function currenciesCommand(options: CommonOptions) { - const { isStdoutPiped } = await import('../utils.js'); const isPiped = isStdoutPiped();
28-28: Inconsistent empty array output format.Same issue as merchants.ts and accounts.ts: writes
'[]\n'directly instead of using consistent JSON formatting.src/mock-pb.ts (1)
13-20: Consider contributing to investec-pb-api to export PayMultiple.The inline copy of
PayMultiplecreates a maintenance burden if the upstream type evolves. Consider opening an issue or PR with theinvestec-pb-apilibrary to export this type.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (60)
.biomeignore(1 hunks).cursorrules(1 hunks).github/copilot-instructions.md(1 hunks)CI_CD_EXAMPLES.md(1 hunks)GENERATED_README.md(1 hunks)README.md(13 hunks)TEST_SUGGESTIONS.md(1 hunks)biome.json(1 hunks)fetched.js(1 hunks)package.json(2 hunks)prettier.config.ts(0 hunks)src/cmds/accounts.ts(1 hunks)src/cmds/ai.ts(2 hunks)src/cmds/balances.ts(1 hunks)src/cmds/bank.ts(3 hunks)src/cmds/beneficiaries.ts(1 hunks)src/cmds/cards.ts(1 hunks)src/cmds/countries.ts(1 hunks)src/cmds/currencies.ts(1 hunks)src/cmds/deploy.ts(1 hunks)src/cmds/disable.ts(1 hunks)src/cmds/docs.ts(1 hunks)src/cmds/env-list.ts(1 hunks)src/cmds/env.ts(1 hunks)src/cmds/fetch.ts(1 hunks)src/cmds/index.ts(2 hunks)src/cmds/login.ts(2 hunks)src/cmds/logs.ts(1 hunks)src/cmds/merchants.ts(1 hunks)src/cmds/new.ts(1 hunks)src/cmds/pay.ts(1 hunks)src/cmds/publish.ts(1 hunks)src/cmds/published.ts(1 hunks)src/cmds/register.ts(1 hunks)src/cmds/run.ts(2 hunks)src/cmds/set.ts(3 hunks)src/cmds/simulate.ts(2 hunks)src/cmds/toggle.ts(1 hunks)src/cmds/transactions.ts(2 hunks)src/cmds/transfer.ts(1 hunks)src/cmds/types.ts(1 hunks)src/cmds/upload-env.ts(1 hunks)src/cmds/upload.ts(1 hunks)src/errors.ts(1 hunks)src/function-calls.ts(2 hunks)src/index.ts(2 hunks)src/mock-card.ts(10 hunks)src/mock-pb.ts(8 hunks)src/utils.ts(2 hunks)templates/default/main.js(1 hunks)templates/petro/main.js(2 hunks)test/cmds/accounts.test.ts(1 hunks)test/cmds/balances.test.ts(1 hunks)test/cmds/cards.test.ts(2 hunks)test/cmds/deploy.test.ts(1 hunks)test/cmds/fetch.test.ts(1 hunks)test/cmds/publish.test.ts(1 hunks)test/cmds/transactions.test.ts(1 hunks)test/cmds/upload.test.ts(1 hunks)vitest.config.ts(1 hunks)
💤 Files with no reviewable changes (1)
- prettier.config.ts
🧰 Additional context used
🧬 Code graph analysis (43)
test/cmds/fetch.test.ts (4)
src/index.ts (1)
credentials(89-97)src/utils.ts (1)
initializeApi(2431-2458)src/cmds/fetch.ts (1)
fetchCommand(24-70)src/errors.ts (1)
CliError(2-9)
test/cmds/transactions.test.ts (2)
src/utils.ts (2)
initializePbApi(2364-2391)formatOutput(1161-1222)src/cmds/transactions.ts (1)
transactionsCommand(27-107)
test/cmds/balances.test.ts (3)
src/index.ts (1)
credentials(89-97)src/utils.ts (1)
initializePbApi(2364-2391)src/cmds/balances.ts (1)
balancesCommand(18-86)
src/cmds/simulate.ts (2)
src/utils.ts (3)
normalizeCardKey(2400-2422)validateFilePath(1465-1482)initializeApi(2431-2458)src/index.ts (1)
credentials(89-97)
src/cmds/env.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (4)
normalizeCardKey(2400-2422)createSpinner(2336-2355)initializeApi(2431-2458)validateFilePathForWrite(1491-1508)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)
src/cmds/pay.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (5)
validateAccountId(1544-1565)validateAmount(1516-1537)initializePbApi(2364-2391)confirmDestructiveOperation(1573-1603)withRetry(2229-2289)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/cmds/run.ts (3)
src/index.ts (1)
printTitleBox(78-80)src/utils.ts (4)
validateFilePath(1465-1482)getFileSize(2318-2321)createSpinner(2336-2355)formatFileSize(2296-2310)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)
src/cmds/transactions.ts (4)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (7)
isStdoutPiped(1112-1114)readStdin(1128-1152)validateAccountId(1544-1565)createSpinner(2336-2355)initializePbApi(2364-2391)withRetry(2229-2289)formatOutput(1161-1222)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/cmds/publish.ts (4)
src/cmds/types.ts (1)
CommonOptions(2-15)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)src/utils.ts (7)
validateFilePath(1465-1482)normalizeCardKey(2400-2422)confirmDestructiveOperation(1573-1603)createSpinner(2336-2355)initializeApi(2431-2458)getFileSize(2318-2321)formatFileSize(2296-2310)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)
src/cmds/balances.ts (4)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (7)
isStdoutPiped(1112-1114)readStdin(1128-1152)validateAccountId(1544-1565)createSpinner(2336-2355)initializePbApi(2364-2391)withRetry(2229-2289)formatOutput(1161-1222)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
test/cmds/deploy.test.ts (4)
src/index.ts (1)
credentials(89-97)src/utils.ts (3)
initializeApi(2431-2458)validateFilePath(1465-1482)confirmDestructiveOperation(1573-1603)src/cmds/deploy.ts (1)
deployCommand(30-109)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)
src/cmds/countries.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (4)
isStdoutPiped(1112-1114)createSpinner(2336-2355)initializeApi(2431-2458)formatOutput(1161-1222)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/cmds/docs.ts (2)
src/index.ts (1)
printTitleBox(78-80)src/utils.ts (1)
validateFilePathForWrite(1491-1508)
src/cmds/accounts.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (6)
isStdoutPiped(1112-1114)createSpinner(2336-2355)initializePbApi(2364-2391)getVerboseMode(61-68)withRetry(2229-2289)formatOutput(1161-1222)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/cmds/upload.ts (2)
src/utils.ts (6)
validateFilePath(1465-1482)normalizeCardKey(2400-2422)createSpinner(2336-2355)initializeApi(2431-2458)getFileSize(2318-2321)formatFileSize(2296-2310)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)
src/cmds/merchants.ts (4)
src/cmds/index.ts (1)
merchantsCommand(39-39)src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (4)
isStdoutPiped(1112-1114)createSpinner(2336-2355)initializeApi(2431-2458)formatOutput(1161-1222)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/cmds/env-list.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/index.ts (1)
printTitleBox(78-80)src/utils.ts (1)
formatOutput(1161-1222)
src/cmds/toggle.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (3)
normalizeCardKey(2400-2422)createSpinner(2336-2355)initializeApi(2431-2458)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)
templates/petro/main.js (1)
templates/default/main.js (3)
_beforeTransaction(2-4)_afterTransaction(6-8)_afterDecline(10-12)
test/cmds/cards.test.ts (2)
src/utils.ts (2)
initializeApi(2431-2458)formatOutput(1161-1222)src/cmds/cards.ts (1)
cardsCommand(10-56)
templates/default/main.js (1)
templates/petro/main.js (3)
_beforeTransaction(8-14)_afterTransaction(17-19)_afterDecline(22-24)
src/cmds/beneficiaries.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (4)
isStdoutPiped(1112-1114)createSpinner(2336-2355)initializePbApi(2364-2391)formatOutput(1161-1222)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/cmds/fetch.ts (3)
src/utils.ts (6)
normalizeCardKey(2400-2422)createSpinner(2336-2355)initializeApi(2431-2458)validateFilePathForWrite(1491-1508)formatFileSize(2296-2310)getFileSize(2318-2321)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)
src/cmds/upload-env.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (4)
validateFilePath(1465-1482)normalizeCardKey(2400-2422)createSpinner(2336-2355)initializeApi(2431-2458)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)
test/cmds/accounts.test.ts (3)
src/index.ts (1)
credentials(89-97)src/utils.ts (2)
initializePbApi(2364-2391)formatOutput(1161-1222)src/cmds/accounts.ts (1)
accountsCommand(15-72)
test/cmds/publish.test.ts (4)
src/index.ts (1)
credentials(89-97)src/utils.ts (2)
initializeApi(2431-2458)validateFilePath(1465-1482)src/cmds/publish.ts (1)
publishCommand(26-64)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)
src/cmds/currencies.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (4)
isStdoutPiped(1112-1114)createSpinner(2336-2355)initializeApi(2431-2458)formatOutput(1161-1222)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/cmds/logs.ts (4)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (6)
normalizeCardKey(2400-2422)createSpinner(2336-2355)initializeApi(2431-2458)validateFilePathForWrite(1491-1508)formatFileSize(2296-2310)getFileSize(2318-2321)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)
test/cmds/upload.test.ts (4)
src/index.ts (1)
credentials(89-97)src/utils.ts (2)
initializeApi(2431-2458)validateFilePath(1465-1482)src/cmds/upload.ts (1)
uploadCommand(23-45)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)
src/cmds/login.ts (3)
src/index.ts (2)
printTitleBox(78-80)credentialLocation(70-73)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)src/utils.ts (3)
readCredentialsFile(1647-1661)ensureCredentialsDirectory(1667-1673)writeCredentialsFile(1844-1850)
src/cmds/deploy.ts (4)
src/cmds/types.ts (1)
CommonOptions(2-15)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)src/utils.ts (8)
validateFilePath(1465-1482)normalizeCardKey(2400-2422)confirmDestructiveOperation(1573-1603)createSpinner(2336-2355)getFileSize(2318-2321)getSafeText(347-376)formatFileSize(2296-2310)withRetry(2229-2289)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)
src/cmds/new.ts (3)
src/index.ts (1)
printTitleBox(78-80)src/utils.ts (1)
getSafeText(347-376)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)
src/cmds/set.ts (3)
src/utils.ts (9)
listProfiles(1882-1897)getActiveProfile(1969-1982)readProfile(1905-1924)setActiveProfile(1990-2006)deleteProfile(1947-1963)writeProfile(1931-1940)readCredentialsFile(1647-1661)ensureCredentialsDirectory(1667-1673)writeCredentialsFile(1844-1850)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)src/index.ts (1)
credentialLocation(70-73)
src/cmds/disable.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (4)
normalizeCardKey(2400-2422)confirmDestructiveOperation(1573-1603)createSpinner(2336-2355)initializeApi(2431-2458)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)
src/cmds/transfer.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (6)
validateAccountId(1544-1565)validateAmount(1516-1537)confirmDestructiveOperation(1573-1603)createSpinner(2336-2355)initializePbApi(2364-2391)withRetry(2229-2289)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/cmds/register.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/index.ts (1)
printTitleBox(78-80)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)
src/cmds/ai.ts (3)
src/cmds/index.ts (1)
aiCommand(41-41)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)src/utils.ts (4)
validateFilePathForWrite(1491-1508)createSpinner(2336-2355)formatFileSize(2296-2310)getFileSize(2318-2321)
src/cmds/bank.ts (2)
src/index.ts (2)
printTitleBox(78-80)credentials(89-97)src/function-calls.ts (1)
tools(241-247)
src/cmds/cards.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (6)
isStdoutPiped(1112-1114)createSpinner(2336-2355)initializeApi(2431-2458)getVerboseMode(61-68)withRetry(2229-2289)formatOutput(1161-1222)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/function-calls.ts (4)
src/cmds/types.ts (1)
BasicOptions(27-33)src/mock-pb.ts (5)
getAccounts(88-104)getAccountBalances(74-87)getAccountTransactions(105-132)getBeneficiaries(133-156)transferMultiple(177-195)src/utils.ts (1)
initializePbApi(2364-2391)src/index.ts (1)
credentials(89-97)
src/cmds/published.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (4)
normalizeCardKey(2400-2422)createSpinner(2336-2355)initializeApi(2431-2458)validateFilePathForWrite(1491-1508)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)
src/utils.ts (5)
src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)src/index.ts (3)
credentialLocation(70-73)credentials(89-97)optionCredentials(1353-1389)src/cmds/types.ts (2)
Credentials(17-25)BasicOptions(27-33)src/mock-pb.ts (2)
IPbApi(22-46)PbApi(48-196)src/mock-card.ts (2)
ICardApi(13-36)CardApi(38-320)
src/index.ts (3)
src/utils.ts (18)
configureChalk(32-39)readCredentialsFileSync(1623-1639)getSafeText(347-376)withCommandContext(794-814)listProfiles(1882-1897)getActiveProfile(1969-1982)readProfile(1905-1924)deleteProfile(1947-1963)getProfilePath(1873-1876)openInEditor(150-210)checkForUpdates(1050-1078)showUpdateNotification(1085-1091)isStdoutPiped(1112-1114)getVerboseMode(61-68)warnAboutSecretUsage(495-540)logCommandHistory(957-979)loadProfile(2014-2037)loadCredentialsFile(1707-1736)src/cmds/types.ts (2)
Credentials(17-25)BasicOptions(27-33)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-40)
🪛 ast-grep (0.39.7)
src/utils.ts
[warning] 356-356: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(emoji, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 368-368: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(emoji, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 LanguageTool
.github/copilot-instructions.md
[style] ~47-~47: Consider using a different verb for a more formal wording.
Context: ...sages**: Errors include suggestions for fixing common issues ```typescript // Example...
(FIX_RESOLVE)
CI_CD_EXAMPLES.md
[uncategorized] ~262-~262: If this is a compound adjective that modifies the following noun, use a hyphen.
Context: ...set DEBUG=1 environment variable ### Rate limiting - Solution: The CLI automatically r...
(EN_COMPOUND_ADJECTIVE_INTERNAL)
GENERATED_README.md
[style] ~328-~328: ‘Makes a payment’ might be wordy. Consider a shorter alternative.
Context: ...y Pay a beneficiary from your account. Makes a payment to a registered beneficiary. Requires c...
(EN_WORDINESS_PREMIUM_MAKES_A_PAYMENT)
README.md
[style] ~235-~235: Consider using “impractical” to avoid wordiness.
Context: ...ry use cases where credential files are not practical - When you understand the security impl...
(NOT_ABLE_PREMIUM)
[style] ~389-~389: ‘Makes a payment’ might be wordy. Consider a shorter alternative.
Context: ...nsfers money between accounts - pay - Makes a payment to a beneficiary **Skip confirmation w...
(EN_WORDINESS_PREMIUM_MAKES_A_PAYMENT)
[style] ~701-~701: ‘make a payment’ might be wordy. Consider a shorter alternative.
Context: ...mand requires confirmation** as it will make a payment from your account. ```sh ipb pay <acco...
(EN_WORDINESS_PREMIUM_MAKE_A_PAYMENT)
🪛 markdownlint-cli2 (0.18.1)
GENERATED_README.md
8-8: Link fragments should be valid
(MD051, link-fragments)
9-9: Link fragments should be valid
(MD051, link-fragments)
10-10: Link fragments should be valid
(MD051, link-fragments)
11-11: Link fragments should be valid
(MD051, link-fragments)
12-12: Link fragments should be valid
(MD051, link-fragments)
13-13: Link fragments should be valid
(MD051, link-fragments)
14-14: Link fragments should be valid
(MD051, link-fragments)
19-19: Link fragments should be valid
(MD051, link-fragments)
26-26: Link fragments should be valid
(MD051, link-fragments)
27-27: Link fragments should be valid
(MD051, link-fragments)
30-30: Link fragments should be valid
(MD051, link-fragments)
README.md
204-204: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
315-315: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (15)
.biomeignore (1)
1-2: LGTM!The ignore patterns are appropriate for excluding build artifacts (
bin/) and generated files (ai-generated.js) from Biome linting and formatting checks.templates/petro/main.js (1)
6-6: LGTM - Code style improvement.Converting to single quotes aligns with the project's code style guidelines.
vitest.config.ts (1)
1-14: LGTM!The Vitest configuration is well-structured with appropriate settings for the project:
- Node environment for CLI testing
- Sensible test discovery patterns
- V8 coverage provider with multiple reporter formats
- Proper exclusions for non-source files
CI_CD_EXAMPLES.md (1)
1-294: LGTM - Comprehensive CI/CD documentation.This documentation provides excellent coverage of deployment scenarios across multiple CI/CD platforms, including:
- Clear examples for GitHub Actions, GitLab CI, and Jenkins
- Environment variable and profile-based credential management
- Security best practices and troubleshooting guidance
- Multi-environment deployment patterns
The examples are practical and align well with the CLI's capabilities described in the PR.
.github/copilot-instructions.md (1)
1-248: LGTM - Excellent contributor documentation.This comprehensive documentation provides clear guidance on:
- Project architecture and technology stack
- Consistent error handling patterns with
CliErrorand exit codes- Command structure and utility function usage
- Testing best practices with Vitest
- Coding standards enforced by Biome
- Development workflow and common patterns
This will significantly improve contributor onboarding and ensure consistency across the codebase.
TEST_SUGGESTIONS.md (1)
1-252: LGTM - Comprehensive test planning documentation.This document provides excellent guidance for expanding test coverage:
- Clear prioritization of high/medium/low priority test targets
- Detailed test scenarios for each command module
- Reusable test implementation template with proper mocking patterns
- Testing best practices aligned with Vitest framework
- Realistic coverage goal of 80%+
This will be valuable for tracking test coverage progress and maintaining consistency in test implementations.
src/cmds/logs.ts (1)
24-52: Log export flow LGTM.The credential normalisation, filename validation, and spinner messaging line up cleanly with the shared utilities. Nice polish on the saved-file feedback.
test/cmds/transactions.test.ts (1)
36-83: Coverage addition looks good.Both the happy path and error propagation are exercised, and the shared utility mocks line up with the new command surface.
src/cmds/index.ts (1)
1-43: Export surface matches the new commands.
envListCommandis now exposed and the AI command alias is clean. No issues here.src/cmds/set.ts (1)
40-162: Profile management flow looks consistentThe new profile list/show/set/delete paths line up cleanly with the shared utilities and error handling. Thank you for tightening the UX around configuration.
src/mock-pb.ts (5)
36-38: Verify the need for both optional and nullable parameter types.The parameters
fromDate?: string | nulluse both the optional marker (?) and explicit| null. This means parameters can beundefinedORnullORstring. If this distinction is intentional and required by the actual API, this is correct. Otherwise, consider using justfromDate?: stringto simplify.
55-73: LGTM!Constructor and token mock implementations are clean and appropriate for testing purposes.
105-132: LGTM!The
getAccountTransactionsmock correctly handles optional parameters and returns properly typed data without requiring type assertions.
133-156: LGTM!The
getBeneficiariesmock provides comprehensive test data with all required fields properly populated.
157-195: LGTM!Both
payMultipleandtransferMultiplemethods correctly handle single-item or array inputs and return appropriate mock responses.
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (6)
src/cmds/pay.ts (1)
39-41: Past issue resolved - beneficiary validation now uses CliError.The beneficiary ID validation now correctly throws
CliErrorinstead of a plainError, maintaining consistency with other validation errors in this command.src/cmds/deploy.ts (1)
31-32: Past issue resolved - spinner now disabled when piped.The spinner disable logic now correctly checks both
options.spinnerandisPiped, preventing spinner escape sequences from polluting piped output.Also applies to: 54-54
test/cmds/cards.test.ts (1)
68-75: Past issue resolved - yaml key included in expectation.The
formatOutputexpectation now includes theyamlkey, correctly matching the actual function call signature fromcardsCommand.src/cmds/upload-env.ts (1)
23-23: Past issue resolved - spinner now started.The spinner is now properly started with
.start(), ensuring progress feedback is displayed to users.src/cmds/disable.ts (1)
36-47: Past issue resolved - spinner cleanup guaranteed.The try/finally block now ensures the spinner is always stopped, even when API calls fail, preventing terminal state corruption.
src/function-calls.ts (1)
177-184: Past issue has been resolved.The previously flagged hardcoded date
'2025-05-24'has been correctly replaced with the validatedfromDatevariable. The function now properly passes through the caller's date range to the API.
🧹 Nitpick comments (10)
snap/snapcraft.yaml (1)
28-35: Clarify architecture support.The snap sources
investec-ipb-linux-x64, which is x64-specific. According to DISTRIBUTION.md, binaries are built for both x64 and arm64 Linux. Either:
- Build architecture-specific snaps (one for x64, one for arm64), or
- Document that this snap is x64-only, or
- Use snapcraft's architecture-specific logic to select the correct binary.
Consider documenting the architecture limitation in snap/snapcraft.yaml comments or the DISTRIBUTION.md section on Snap packaging.
DISTRIBUTION.md (2)
286-287: Consider using markdown link syntax for URLs.Static analysis suggests wrapping bare URLs in markdown link syntax for better readability:
-1. **Create Launchpad account** at https://launchpad.net -2. **Create PPA** at https://launchpad.net/people/+new-ppa +1. **Create Launchpad account** at <https://launchpad.net> +2. **Create PPA** at <https://launchpad.net/people/+new-ppa>
332-339: LGTM! Consider adding a version sync checklist.The version management section clearly documents where versions need updates. Consider adding a pre-release checklist or script to verify all versions are in sync (package.json, snap/snapcraft.yaml, Formula/ipb.rb).
scripts/create-deb.sh (2)
31-38: Simplify redundant architecture logic.Lines 32-35 contain redundant logic. When
ARCH=arm64, line 32 already setsBINARY_SOURCE="dist/investec-ipb-linux-arm64", so the conditional check on lines 33-35 is unnecessary.Apply this simplification:
# Copy binary BINARY_SOURCE="dist/investec-ipb-linux-${ARCH}" -if [ "$ARCH" = "arm64" ]; then - BINARY_SOURCE="dist/investec-ipb-linux-arm64" -fi - cp "${BINARY_SOURCE}" "${PACKAGE_DIR}/usr/local/bin/ipb" chmod +x "${PACKAGE_DIR}/usr/local/bin/ipb"
40-43: Consider adding error handling for package.json extraction.If package.json is missing or malformed, the node commands will fail silently or return "undefined". Consider adding validation or error handling.
# Get package info from package.json with validation if [ ! -f "package.json" ]; then echo "Error: package.json not found" exit 1 fi PACKAGE_DESC=$(node -p "require('./package.json').description || 'IPB CLI'") PACKAGE_AUTHOR=$(node -p "require('./package.json').author || 'Unknown'") PACKAGE_HOMEPAGE=$(node -p "require('./package.json').homepage || 'https://github.com/devinpearson/ipb'")src/cmds/bank.ts (1)
13-13: Consider passing OpenAI client as parameter instead of module-level state.The module-level
openaivariable (line 13) creates shared state that can complicate testing and potential concurrent usage. Consider refactoringgenerateResponseandsecondCallto accept the OpenAI client as a parameter.This would make the functions more testable and eliminate the module-level mutable state:
async function generateResponse( prompt: string, instructions: string, client: OpenAI ): Promise<string | null> { // ... use client instead of openai }src/cmds/transfer.ts (1)
71-71: Consider disabling spinner when stdout is piped.Unlike
deploy.ts(lines 31-32, 54), this command doesn't check if stdout is piped when determining whether to disable the spinner. This could result in spinner escape sequences polluting piped output.Apply this diff to align with the pattern in
deploy.ts:+ const { isStdoutPiped } = await import('../utils.js'); + const isPiped = isStdoutPiped(); + - const disableSpinner = options.spinner === true; + const disableSpinner = options.spinner === true || isPiped;.github/workflows/release.yml (1)
150-151: Update action version to v2.The
softprops/action-gh-release@v1action uses a runner that is too old for current GitHub Actions. Upgrade tov2for better compatibility and security.Apply this diff:
- name: Create GitHub Release - uses: softprops/action-gh-release@v1 + uses: softprops/action-gh-release@v2src/cmds/upload-env.ts (1)
22-22: Consider disabling spinner when stdout is piped.This command doesn't check if stdout is piped when determining whether to disable the spinner, unlike
deploy.ts(lines 31-32, 54). This inconsistency could result in spinner escape sequences polluting piped output.Apply this diff to align with the pattern in
deploy.ts:+ const { isStdoutPiped } = await import('../utils.js'); + const isPiped = isStdoutPiped(); printTitleBox(); - const disableSpinner = options.spinner === true; + const disableSpinner = options.spinner === true || isPiped;src/cmds/disable.ts (1)
34-35: Consider disabling spinner when stdout is piped.This command doesn't check if stdout is piped when determining whether to disable the spinner, unlike
deploy.ts(lines 31-32, 54). This inconsistency could result in spinner escape sequences polluting piped output.Apply this diff to align with the pattern in
deploy.ts:+ const { isStdoutPiped } = await import('../utils.js'); + const isPiped = isStdoutPiped(); + const disableSpinner = options.spinner === true; // default false - const spinner = createSpinner(!disableSpinner, '🍄 disabling code on card...').start(); + const spinner = createSpinner(options.spinner === true || isPiped, '🍄 disabling code on card...').start();Wait, let me fix that - I need to update the logic properly:
+ const { isStdoutPiped } = await import('../utils.js'); + const isPiped = isStdoutPiped(); + - const disableSpinner = options.spinner === true; // default false + const disableSpinner = options.spinner === true || isPiped; const spinner = createSpinner(!disableSpinner, '🍄 disabling code on card...').start();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (33)
.github/workflows/release.yml(1 hunks).gitignore(1 hunks)DISTRIBUTION.md(1 hunks)Formula/ipb.rb(1 hunks)README.md(14 hunks)UBUNTU_DISTRIBUTION.md(1 hunks)esbuild.config.js(1 hunks)package.json(2 hunks)pkg.config.json(1 hunks)scripts/create-deb.sh(1 hunks)scripts/update-homebrew.sh(1 hunks)snap/snapcraft.yaml(1 hunks)src/cmds/ai.ts(2 hunks)src/cmds/balances.ts(1 hunks)src/cmds/bank.ts(3 hunks)src/cmds/deploy.ts(1 hunks)src/cmds/disable.ts(1 hunks)src/cmds/docs.ts(1 hunks)src/cmds/login.ts(2 hunks)src/cmds/new.ts(1 hunks)src/cmds/pay.ts(1 hunks)src/cmds/publish.ts(1 hunks)src/cmds/published.ts(1 hunks)src/cmds/run.ts(2 hunks)src/cmds/transactions.ts(3 hunks)src/cmds/transfer.ts(1 hunks)src/cmds/upload-env.ts(1 hunks)src/errors.ts(1 hunks)src/function-calls.ts(2 hunks)src/utils.ts(2 hunks)templates/petro/main.js(1 hunks)test/cmds/accounts.test.ts(1 hunks)test/cmds/cards.test.ts(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🚧 Files skipped from review as they are similar to previous changes (4)
- templates/petro/main.js
- src/cmds/new.ts
- src/cmds/docs.ts
- src/errors.ts
🧰 Additional context used
🧬 Code graph analysis (17)
src/cmds/ai.ts (4)
src/cmds/index.ts (1)
aiCommand(41-41)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)src/utils.ts (4)
validateFilePathForWrite(1516-1533)createSpinner(2363-2390)formatFileSize(2321-2335)getFileSize(2343-2346)
src/cmds/transfer.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (6)
validateAccountId(1569-1590)validateAmount(1541-1562)confirmDestructiveOperation(1598-1628)createSpinner(2363-2390)initializePbApi(2399-2426)withRetry(2254-2314)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/cmds/deploy.ts (4)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (9)
isStdoutPiped(1137-1139)validateFilePath(1490-1507)confirmDestructiveOperation(1598-1628)createSpinner(2363-2390)getFileSize(2343-2346)getSafeText(372-401)formatFileSize(2321-2335)getVerboseMode(61-68)withRetry(2254-2314)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)
src/cmds/publish.ts (4)
src/cmds/types.ts (1)
CommonOptions(2-15)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)src/utils.ts (7)
validateFilePath(1490-1507)normalizeCardKey(2435-2457)confirmDestructiveOperation(1598-1628)createSpinner(2363-2390)initializeApi(2466-2493)getFileSize(2343-2346)formatFileSize(2321-2335)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)
test/cmds/cards.test.ts (3)
src/index.ts (1)
credentials(89-97)src/utils.ts (2)
initializeApi(2466-2493)formatOutput(1186-1247)src/cmds/cards.ts (1)
cardsCommand(10-56)
src/cmds/transactions.ts (4)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (7)
isStdoutPiped(1137-1139)readStdin(1153-1177)validateAccountId(1569-1590)createSpinner(2363-2390)initializePbApi(2399-2426)withRetry(2254-2314)formatOutput(1186-1247)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/cmds/disable.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (4)
normalizeCardKey(2435-2457)confirmDestructiveOperation(1598-1628)createSpinner(2363-2390)initializeApi(2466-2493)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)
src/cmds/pay.ts (4)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (5)
validateAccountId(1569-1590)validateAmount(1541-1562)initializePbApi(2399-2426)confirmDestructiveOperation(1598-1628)withRetry(2254-2314)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
test/cmds/accounts.test.ts (2)
src/utils.ts (2)
initializePbApi(2399-2426)formatOutput(1186-1247)src/cmds/accounts.ts (1)
accountsCommand(15-72)
src/cmds/login.ts (3)
src/index.ts (1)
credentialLocation(70-73)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)src/utils.ts (4)
normalizeFilePath(1394-1406)ensureCredentialsDirectory(1692-1698)readCredentialsFile(1672-1686)writeCredentialsFile(1869-1875)
src/cmds/run.ts (3)
src/index.ts (1)
printTitleBox(78-80)src/utils.ts (4)
validateFilePath(1490-1507)getFileSize(2343-2346)createSpinner(2363-2390)formatFileSize(2321-2335)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)
src/cmds/balances.ts (4)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (7)
isStdoutPiped(1137-1139)readStdin(1153-1177)validateAccountId(1569-1590)createSpinner(2363-2390)initializePbApi(2399-2426)withRetry(2254-2314)formatOutput(1186-1247)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
src/cmds/bank.ts (3)
src/index.ts (2)
printTitleBox(78-80)credentials(89-97)src/function-calls.ts (1)
tools(242-248)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)
src/cmds/upload-env.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (4)
validateFilePath(1490-1507)normalizeCardKey(2435-2457)createSpinner(2363-2390)initializeApi(2466-2493)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)
src/cmds/published.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (4)
normalizeCardKey(2435-2457)createSpinner(2363-2390)initializeApi(2466-2493)validateFilePathForWrite(1516-1533)src/index.ts (2)
credentials(89-97)printTitleBox(78-80)
src/function-calls.ts (4)
src/cmds/types.ts (1)
BasicOptions(27-33)src/mock-pb.ts (5)
getAccounts(88-104)getAccountBalances(74-87)getAccountTransactions(105-132)getBeneficiaries(133-156)transferMultiple(177-195)src/utils.ts (1)
initializePbApi(2399-2426)src/index.ts (1)
credentials(89-97)
src/utils.ts (5)
src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)src/index.ts (3)
credentialLocation(70-73)credentials(89-97)optionCredentials(1353-1389)src/cmds/types.ts (2)
Credentials(17-25)BasicOptions(27-33)src/mock-pb.ts (2)
IPbApi(22-46)PbApi(48-196)src/mock-card.ts (2)
ICardApi(13-36)CardApi(38-320)
🪛 actionlint (1.7.8)
.github/workflows/release.yml
126-126: shellcheck reported issue in this script: SC2086:info:2:28: Double quote to prevent globbing and word splitting
(shellcheck)
138-138: shellcheck reported issue in this script: SC2129:style:6:5: Consider using { cmd1; cmd2; } >> file instead of individual redirects
(shellcheck)
151-151: the runner of "softprops/action-gh-release@v1" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 ast-grep (0.39.7)
src/utils.ts
[warning] 381-381: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(emoji, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 393-393: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(emoji, 'g')
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🪛 Brakeman (7.1.0)
Formula/ipb.rb
[medium] 64-64: Possible command injection
Type: Command Injection
Confidence: Medium
More info: https://brakemanscanner.org/docs/warning_types/command_injection/
(Command Injection)
🪛 LanguageTool
UBUNTU_DISTRIBUTION.md
[uncategorized] ~243-~243: The official name of this software platform is spelled with a capital “H”.
Context: ... ## GitHub Actions Integration Add to .github/workflows/release.yml to automatically...
(GITHUB)
README.md
[style] ~306-~306: Consider using “impractical” to avoid wordiness.
Context: ...ry use cases where credential files are not practical - When you understand the security impl...
(NOT_ABLE_PREMIUM)
[style] ~460-~460: ‘Makes a payment’ might be wordy. Consider a shorter alternative.
Context: ...nsfers money between accounts - pay - Makes a payment to a beneficiary **Skip confirmation w...
(EN_WORDINESS_PREMIUM_MAKES_A_PAYMENT)
[style] ~772-~772: ‘make a payment’ might be wordy. Consider a shorter alternative.
Context: ...mand requires confirmation** as it will make a payment from your account. ```sh ipb pay <acco...
(EN_WORDINESS_PREMIUM_MAKE_A_PAYMENT)
DISTRIBUTION.md
[uncategorized] ~180-~180: The official name of this software platform is spelled with a capital “H”.
Context: ...ase binaries when tags are pushed. See .github/workflows/release.yml for the automate...
(GITHUB)
[uncategorized] ~388-~388: The official name of this software platform is spelled with a capital “H”.
Context: ...or downloads ## CI/CD Integration See .github/workflows/release.yml for automated re...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
UBUNTU_DISTRIBUTION.md
78-78: Bare URL used
(MD034, no-bare-urls)
80-80: Bare URL used
(MD034, no-bare-urls)
174-174: Bare URL used
(MD034, no-bare-urls)
README.md
275-275: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
386-386: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
DISTRIBUTION.md
286-286: Bare URL used
(MD034, no-bare-urls)
287-287: Bare URL used
(MD034, no-bare-urls)
🔇 Additional comments (11)
Formula/ipb.rb (2)
23-26: LGTM! Placeholder checksums will be replaced during release.The placeholder SHA256 values are expected. The
scripts/update-homebrew.shscript automates replacing these with actual checksums. Ensure this script is run before publishing the formula.
63-65: LGTM! Command injection warning is a false positive.The test block correctly verifies the binary. The static analysis warning about command injection is a false positive—Homebrew's
#{bin}/ipbis a safe interpolation pattern referencing a controlled installation path.src/cmds/published.ts (2)
22-39: LGTM! Spinner handling is properly implemented.The spinner lifecycle is correctly managed with try/finally, addressing the past review comment. The spinner is guaranteed to stop whether the API call succeeds or throws, preventing terminal corruption.
1-39: Inconsistency: PR contains functional code changes, not just documentation.The PR objectives state "This pull request contains documentation and configuration changes only; no functional code changes." However, this file contains functional code changes including:
- Updated imports and error handling
- Refactored cardKey normalization
- Enhanced spinner management
- Promise-based file operations
The AI summary correctly identifies this as a "Comprehensive CLI refactor" with functional changes.
src/cmds/ai.ts (2)
113-122: LGTM! OpenAI response validation properly implemented.The validation guard addresses the past review comment by checking for missing or invalid code before using
response.code. This prevents the TypeError and provides clear error messaging through CliError.
129-161: LGTM! Spinners are properly managed with try/finally.Both file write operations correctly wrap spinner usage in try/finally blocks, ensuring spinners stop on both success and error paths.
src/cmds/bank.ts (1)
91-98: LGTM! Error handling properly rethrows instead of swallowing.The error handling addresses the past review comment by rethrowing errors as CliError instead of returning null. This ensures failures are surfaced to the CLI runner with proper error codes.
DISTRIBUTION.md (1)
58-65: Binary naming convention verified; documentation is accurate.The package.json name is
investec-ipb, which matches the documented binary naming convention in DISTRIBUTION.md. No issues found.snap/snapcraft.yaml (1)
1-2: Version sync is already documented and currently in sync.The versions in
snap/snapcraft.yaml(0.8.3) andpackage.json(0.8.3) match. The version management requirement is already documented inDISTRIBUTION.md, which explicitly lists "Update version in snap/snapcraft.yaml" as part of the release process. No action needed.esbuild.config.js (1)
1-114: LGTM! Well-structured build configuration.The esbuild configuration follows best practices:
- Promise-based file operations with comprehensive error handling
- Defensive shebang insertion that avoids duplicates
- Clear progress logging for asset copying
- Proper directory creation with
recursive: true- Clean export for external usage
.github/workflows/release.yml (1)
124-129: Quote the version variable to prevent word splitting.The
$VERSIONvariable should be quoted to prevent globbing and word splitting, as flagged by shellcheck (SC2086).Apply this diff:
- name: Extract version from tag id: tag_version run: | VERSION=${GITHUB_REF#refs/tags/v} - echo "version=$VERSION" >> $GITHUB_OUTPUT - echo "Version: $VERSION" + echo "version=$VERSION" >> $GITHUB_OUTPUT + echo "Version: $VERSION"Likely an incorrect or invalid review comment.
| BINARIES=( | ||
| "dist/ipb-macos-x64" | ||
| "dist/ipb-macos-arm64" | ||
| "dist/ipb-linux-x64" | ||
| "dist/ipb-linux-arm64" | ||
| "dist/ipb-win-x64.exe" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
Verify binary naming consistency.
The binary names in this script use the ipb- prefix (e.g., dist/ipb-macos-x64), but DISTRIBUTION.md documents them as investec-ipb- prefix (e.g., dist/investec-ipb-macos-x64). This mismatch will cause the script to fail if the actual binaries use the investec-ipb- prefix.
🏁 Script executed:
#!/bin/bash
# Check what binaries actually exist in dist/ or are created by build
echo "Checking package.json for binary configuration..."
if [ -f "package.json" ]; then
node -p "require('./package.json').name"
node -p "JSON.stringify(require('./package.json').pkg || {}, null, 2)"
fi
echo ""
echo "Checking if dist/ directory exists and what files it contains..."
if [ -d "dist" ]; then
fd -t f . dist/
else
echo "dist/ directory not found"
fiLength of output: 489
🏁 Script executed:
#!/bin/bash
# Find and check DISTRIBUTION.md for binary naming info
echo "Searching for DISTRIBUTION.md..."
fd -t f "DISTRIBUTION.md" .
echo ""
echo "Checking for any documentation about binary naming..."
fd -t f "README.md" . | head -5
echo ""
echo "Searching for references to binary names in scripts/update-homebrew.sh..."
if [ -f "scripts/update-homebrew.sh" ]; then
cat scripts/update-homebrew.sh | head -50
fiLength of output: 1450
🏁 Script executed:
#!/bin/bash
# Read DISTRIBUTION.md to verify what binary names are documented
cat DISTRIBUTION.mdLength of output: 9840
Fix binary names in BINARIES array to match actual output.
The binary names in lines 38-44 use the ipb- prefix, but according to DISTRIBUTION.md and package.json configuration, they should use the investec-ipb- prefix. Update all five binary names:
BINARIES=(
"dist/investec-ipb-macos-x64"
"dist/investec-ipb-macos-arm64"
"dist/investec-ipb-linux-x64"
"dist/investec-ipb-linux-arm64"
"dist/investec-ipb-win-x64.exe"
)
🤖 Prompt for AI Agents
In scripts/update-homebrew.sh around lines 38 to 44, the BINARIES array uses the
incorrect "ipb-" prefix for output filenames; update all five entries to use the
correct "investec-ipb-" prefix (matching DISTRIBUTION.md and package.json) so
the array becomes: dist/investec-ipb-macos-x64, dist/investec-ipb-macos-arm64,
dist/investec-ipb-linux-x64, dist/investec-ipb-linux-arm64, and
dist/investec-ipb-win-x64.exe.
| # Update checksum in formula | ||
| if [[ "$OSTYPE" == "darwin"* ]]; then | ||
| sed -i '' "s|url \".*$binary_name\"|url \"https://github.com/devinpearson/ipb/releases/download/v$VERSION/$binary_name\"|" "$FORMULA_FILE" | ||
| sed -i '' "s|sha256 \".*\" # $binary_name|sha256 \"$checksum\" # $binary_name|" "$FORMULA_FILE" | ||
| else | ||
| sed -i "s|url \".*$binary_name\"|url \"https://github.com/devinpearson/ipb/releases/download/v$VERSION/$binary_name\"|" "$FORMULA_FILE" | ||
| sed -i "s|sha256 \".*\" # $binary_name|sha256 \"$checksum\" # $binary_name|" "$FORMULA_FILE" | ||
| fi |
There was a problem hiding this comment.
Fix sed pattern mismatch with formula structure.
The sed patterns attempt to match sha256 ".*" # $binary_name, but Formula/ipb.rb doesn't have binary names in comments after sha256 lines. The formula only has comments like # Calculate SHA256: shasum -a 256 dist/ipb-macos-arm64. This will cause the checksum updates to fail.
Update the sed patterns to match the actual formula structure or add binary name markers to the formula. For example:
- sed -i '' "s|sha256 \".*\" # $binary_name|sha256 \"$checksum\" # $binary_name|" "$FORMULA_FILE"
+ # Update the sha256 line after the matching URL
+ sed -i '' "/url \".*$binary_name\"/,/sha256/s|sha256 \".*\"|sha256 \"$checksum\"|" "$FORMULA_FILE"📝 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.
| # Update checksum in formula | |
| if [[ "$OSTYPE" == "darwin"* ]]; then | |
| sed -i '' "s|url \".*$binary_name\"|url \"https://github.com/devinpearson/ipb/releases/download/v$VERSION/$binary_name\"|" "$FORMULA_FILE" | |
| sed -i '' "s|sha256 \".*\" # $binary_name|sha256 \"$checksum\" # $binary_name|" "$FORMULA_FILE" | |
| else | |
| sed -i "s|url \".*$binary_name\"|url \"https://github.com/devinpearson/ipb/releases/download/v$VERSION/$binary_name\"|" "$FORMULA_FILE" | |
| sed -i "s|sha256 \".*\" # $binary_name|sha256 \"$checksum\" # $binary_name|" "$FORMULA_FILE" | |
| fi | |
| # Update checksum in formula | |
| if [[ "$OSTYPE" == "darwin"* ]]; then | |
| sed -i '' "s|url \".*$binary_name\"|url \"https://github.com/devinpearson/ipb/releases/download/v$VERSION/$binary_name\"|" "$FORMULA_FILE" | |
| # Update the sha256 line after the matching URL | |
| sed -i '' "/url \".*$binary_name\"/,/sha256/s|sha256 \".*\"|sha256 \"$checksum\"|" "$FORMULA_FILE" | |
| else | |
| sed -i "s|url \".*$binary_name\"|url \"https://github.com/devinpearson/ipb/releases/download/v$VERSION/$binary_name\"|" "$FORMULA_FILE" | |
| sed -i "s|sha256 \".*\" # $binary_name|sha256 \"$checksum\" # $binary_name|" "$FORMULA_FILE" | |
| fi |
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
src/function-calls.ts (2)
128-129: Consider improving type safety of Options object.All functions pass
{} as OptionstoinitializePbApi, which bypasses TypeScript's type checking for required fields inBasicOptions(host, apiKey, clientId, clientSecret, credentialsFile). While this appears to work becauseoptionCredentialsmerges the credentials, it weakens compile-time guarantees.Consider either:
- Making BasicOptions fields optional if they can be sourced from credentials
- Passing a properly constructed options object with defaults
- Creating a separate interface for API initialization that doesn't require all BasicOptions fields
Also applies to: 147-147, 178-178, 194-194, 228-228
177-182: Optional: Simplify by removing intermediate variable.The
fromDatevariable on line 177 is extracted but immediately used. You could directly useoptions.fromDatein the API call on line 182 for slightly cleaner code.- const fromDate = options.fromDate; const api = await initializePbApi(credentials, {} as Options); console.log( - `💳 fetching transactions for account ${options.accountId}, fromDate: ${fromDate}, toDate: ${options.toDate}` + `💳 fetching transactions for account ${options.accountId}, fromDate: ${options.fromDate}, toDate: ${options.toDate}` ); - const result = await api.getAccountTransactions(options.accountId, fromDate, options.toDate); + const result = await api.getAccountTransactions(options.accountId, options.fromDate, options.toDate);src/cmds/transfer.ts (1)
48-50: Consider validating the reference field.The reference is prompted but not validated. Unlike
accountIdandamount, which undergo validation, an empty or malformed reference could be passed to the API, potentially causing rejection or unclear transaction records.Consider adding basic validation:
if (!reference) { reference = await input({ message: 'Enter reference for the transfer:' }); } +if (!reference || reference.trim().length === 0) { + throw new CliError(ERROR_CODES.INVALID_INPUT, 'Reference is required and cannot be empty'); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
DISTRIBUTION.md(1 hunks)README.md(14 hunks)UBUNTU_DISTRIBUTION.md(1 hunks)scripts/create-deb.sh(1 hunks)scripts/update-homebrew.sh(1 hunks)snap/snapcraft.yaml(1 hunks)src/cmds/run.ts(2 hunks)src/cmds/transfer.ts(1 hunks)src/function-calls.ts(2 hunks)test/cmds/accounts.test.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- test/cmds/accounts.test.ts
- scripts/create-deb.sh
🧰 Additional context used
🧬 Code graph analysis (3)
src/function-calls.ts (4)
src/cmds/types.ts (1)
BasicOptions(27-33)src/mock-pb.ts (5)
getAccounts(88-104)getAccountBalances(74-87)getAccountTransactions(105-132)getBeneficiaries(133-156)transferMultiple(177-195)src/utils.ts (1)
initializePbApi(2399-2426)src/index.ts (1)
credentials(89-97)
src/cmds/run.ts (3)
src/index.ts (1)
printTitleBox(78-80)src/utils.ts (4)
validateFilePath(1490-1507)getFileSize(2343-2346)createSpinner(2363-2390)formatFileSize(2321-2335)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)
src/cmds/transfer.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (6)
validateAccountId(1569-1590)validateAmount(1541-1562)confirmDestructiveOperation(1598-1628)createSpinner(2363-2390)initializePbApi(2399-2426)withRetry(2254-2314)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
🪛 LanguageTool
DISTRIBUTION.md
[uncategorized] ~180-~180: The official name of this software platform is spelled with a capital “H”.
Context: ...ase binaries when tags are pushed. See .github/workflows/release.yml for the automate...
(GITHUB)
[uncategorized] ~388-~388: The official name of this software platform is spelled with a capital “H”.
Context: ...or downloads ## CI/CD Integration See .github/workflows/release.yml for automated re...
(GITHUB)
README.md
[style] ~306-~306: Consider using “impractical” to avoid wordiness.
Context: ...ry use cases where credential files are not practical - When you understand the security impl...
(NOT_ABLE_PREMIUM)
[style] ~460-~460: ‘Makes a payment’ might be wordy. Consider a shorter alternative.
Context: ...nsfers money between accounts - pay - Makes a payment to a beneficiary **Skip confirmation w...
(EN_WORDINESS_PREMIUM_MAKES_A_PAYMENT)
[style] ~772-~772: ‘make a payment’ might be wordy. Consider a shorter alternative.
Context: ...mand requires confirmation** as it will make a payment from your account. ```sh ipb pay <acco...
(EN_WORDINESS_PREMIUM_MAKE_A_PAYMENT)
UBUNTU_DISTRIBUTION.md
[uncategorized] ~243-~243: The official name of this software platform is spelled with a capital “H”.
Context: ... ## GitHub Actions Integration Add to .github/workflows/release.yml to automatically...
(GITHUB)
🪛 markdownlint-cli2 (0.18.1)
DISTRIBUTION.md
286-286: Bare URL used
(MD034, no-bare-urls)
287-287: Bare URL used
(MD034, no-bare-urls)
README.md
275-275: Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
386-386: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (19)
README.md (5)
1052-1196: Comprehensive testing section is well-structured.The expanded Testing section provides clear guidance on running tests, test structure, patterns, and writing new tests. This aligns well with the PR's goal of improving contributor onboarding and establishing architectural patterns.
205-271: Security and environment configuration guidance is thorough.The new Environment Variables and Security Best Practices subsections (lines 205–271) provide clear, actionable guidance on credential handling, environment variable usage, and the priority order for configuration resolution. The emphasis on credential files over environment variables is sound security practice.
342-445: Configuration Profiles section is well-documented with practical examples.The new Configuration Profiles section clearly explains profile creation, management, and usage with concrete workflow examples. This significantly improves the onboarding experience for multi-environment setups.
451-474: Confirmation for Destructive Operations section is clear and actionable.The new section (lines 451–474) clearly enumerates destructive commands and provides both interactive and non-interactive (
--yes) usage patterns. This is valuable for both manual and automation contexts.
62-178: Installation section now provides multiple distribution paths.The expanded Installation section with npm, Homebrew, direct downloads (for macOS, Linux, Windows), and shell autocomplete guidance significantly improves accessibility and reduces setup friction for new users across different platforms.
snap/snapcraft.yaml (1)
1-42: Snapcraft manifest looks good.The configuration is clean and aligns with the binary naming and output layout described in DISTRIBUTION.md. Ensure that
npm run pkg:linuxis executed before runningsnapcraftto generate thedist/ipb-linux-x64binary that this manifest sources from.scripts/update-homebrew.sh (2)
52-60: Inconsistent sed patterns between platforms may cause Linux failures.The macOS version (line 56) uses an improved range-based sed pattern, but the Linux version (line 59) still uses the original pattern. If the Homebrew formula structure doesn't include binary name comments after sha256 lines, the Linux update will fail silently while macOS succeeds.
Update the Linux sed command to match the macOS pattern for consistency:
- sed -i "s|sha256 \".*\" # $binary_name|sha256 \"$checksum\" # $binary_name|" "$FORMULA_FILE" + sed -i "/url \".*$binary_name\"/,/sha256/s|sha256 \".*\"|sha256 \"$checksum\"|" "$FORMULA_FILE"
38-44: Binary naming is consistent with DISTRIBUTION.md.The BINARIES array naming aligns with the documented output format in DISTRIBUTION.md (lines 59–65), which explicitly states the binaries use the
ipbprefix. No changes needed here.UBUNTU_DISTRIBUTION.md (1)
1-10: Bare URL issue previously flagged appears to be resolved.The URLs mentioned in the past review comments (e.g., lines 78–80) are now properly wrapped in Markdown link format. The documentation looks clean.
DISTRIBUTION.md (1)
1-50: Binary naming clarification in documentation.Lines 59–65 clearly document the binary naming convention with the
ipbprefix, which aligns with the Snapcraft manifest and update-homebrew.sh script. The documentation is consistent and well-explained.src/function-calls.ts (5)
1-256: PR objectives inconsistency.The PR description states "documentation and configuration changes only; no functional code changes," but this file contains substantial functional code modifications including runtime validation, API integration changes, and updated function signatures.
177-182: Past critical issue resolved.The hardcoded date issue has been properly fixed. The function now correctly uses the validated
fromDatefrom options instead of the previously hardcoded '2025-05-24'.
230-235: Past critical issue resolved.The hardcoded amount issue has been properly fixed. The transfer now correctly uses
options.amountinstead of the previously hardcoded test value '10'.
143-146: Good runtime validation pattern.The runtime validation checks provide appropriate defensive programming for the function-calling interface, where arguments originate from the AI model. The error messages are clear and specify the required structure.
Also applies to: 166-176, 215-227
31-36: Excellent documentation additions.The JSDoc comments provide clear documentation for all functions, improving code maintainability and developer experience.
Also applies to: 122-127, 136-141, 154-159, 187-192, 201-206
src/cmds/transfer.ts (4)
1-11: LGTM!All imports are appropriately used in the transfer command implementation.
13-28: LGTM!The function signature is well-documented and correctly typed using
CommonOptions.
52-69: LGTM!The transfer summary display and confirmation flow correctly use
confirmDestructiveOperation, respect the--yesflag, and exit gracefully when the user cancels.
75-99: LGTM!The transfer execution correctly uses
withRetryfor resilience, manages the spinner lifecycle (start → succeed/fail), and logs per-transfer status from the API response. Error handling properly marks the spinner as failed and rethrows for upstream handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (8)
scripts/sign-macos.sh (3)
8-18: Consider validating the signing identity before use.The script defers identity validation until the first signing attempt. For better user experience, consider validating the provided identity against available certificates earlier to catch misconfigurations immediately.
23-47: Refactor duplicated signing logic into a reusable function.The signing logic for x64 (lines 24–34) and ARM64 (lines 37–47) architectures is nearly identical. Extracting this into a helper function would improve maintainability and reduce duplication.
Example refactor:
sign_binary() { local arch=$1 local binary_path=$2 if [ -f "$binary_path" ]; then echo "Signing ipb-macos-${arch}..." codesign --sign "$IDENTITY" \ --options runtime \ --timestamp \ --force \ "$binary_path" echo "✅ Signed ipb-macos-${arch}" else echo "⚠️ $binary_path not found, skipping..." fi } # Usage: sign_binary "x64" "dist/ipb-macos-x64" sign_binary "arm64" "dist/ipb-macos-arm64"
49-57: Clarify output when verifying missing binaries.Lines 52 and 57 will report "signature verification failed" or "No entitlements found" for binaries that may not exist. Consider checking file existence before verification to provide clearer feedback about what was actually checked.
Example improvement for verification:
for arch in "x64" "arm64"; do binary_path="dist/ipb-macos-${arch}" if [ -f "$binary_path" ]; then codesign --verify --verbose "$binary_path" 2>/dev/null && \ echo "✅ ipb-macos-${arch} signature verified" || \ echo "❌ ipb-macos-${arch} signature verification failed" else echo "⚠️ ipb-macos-${arch} not found, skipping verification..." fi donesrc/cmds/run.ts (5)
48-48: Use camelCase convention for variable naming.The variable
environmentvariablesshould follow JavaScript's camelCase convention.Apply this diff:
- let environmentvariables: { [key: string]: string } = {}; + let environmentVariables: { [key: string]: string } = {};Don't forget to update references at lines 71 and 77.
64-67: Consider using isStdout() pattern for spinner consistency.The spinner is hardcoded to
true, but other commands in the codebase useisStdout()to detect piped output and disable spinners accordingly. This ensures clean output when piping to other commands or files.Based on learnings
Consider applying this pattern:
+ const showSpinner = isStdout(); const spinner = createSpinner( - true, + showSpinner, `📖 reading env from ${envFilePath} (${formatFileSize(envFileSize)})...` ).start();Note: You'll need to import
isStdoutfrom utils if not already imported.
77-77: Use camelCase convention for variable naming.The variable
environmentvariablesStringshould follow JavaScript's camelCase convention.Apply this diff:
- const environmentvariablesString = JSON.stringify(environmentvariables); + const environmentVariablesString = JSON.stringify(environmentVariables);Don't forget to update the reference at line 91.
80-83: Consider using isStdout() pattern for spinner consistency.Same as the env file spinner, this is hardcoded to
trueinstead of respecting piped output detection.Based on learnings
Apply the same
isStdout()pattern:+ const showSpinner = isStdout(); const codeSpinner = createSpinner( - true, + showSpinner, `📖 reading code from ${normalizedFilename} (${formatFileSize(codeFileSize)})...` ).start();
105-108: Remove unused variable.The
_txtvariable is assigned but never used.Apply this diff:
if (line !== '\r') { - const _txt = line?.trim(); - if (line) {Or if the intent was to use
_txtfor the subsequent check:if (line !== '\r') { const _txt = line?.trim(); - if (line) { + if (_txt) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
DISTRIBUTION.md(1 hunks)README.md(14 hunks)scripts/sign-macos.sh(1 hunks)src/cmds/run.ts(2 hunks)src/cmds/transfer.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/cmds/run.ts (2)
src/utils.ts (4)
validateFilePath(1490-1507)getFileSize(2343-2346)createSpinner(2363-2390)formatFileSize(2321-2335)src/errors.ts (2)
CliError(2-9)ERROR_CODES(25-41)
src/cmds/transfer.ts (3)
src/cmds/types.ts (1)
CommonOptions(2-15)src/utils.ts (6)
validateAccountId(1569-1590)validateAmount(1541-1562)confirmDestructiveOperation(1598-1628)createSpinner(2363-2390)initializePbApi(2399-2426)withRetry(2254-2314)src/index.ts (2)
printTitleBox(78-80)credentials(89-97)
🪛 LanguageTool
README.md
[style] ~306-~306: Consider using “impractical” to avoid wordiness.
Context: ...ry use cases where credential files are not practical - When you understand the security impl...
(NOT_ABLE_PREMIUM)
[style] ~460-~460: ‘Makes a payment’ might be wordy. Consider a shorter alternative.
Context: ...nsfers money between accounts - pay - Makes a payment to a beneficiary **Skip confirmation w...
(EN_WORDINESS_PREMIUM_MAKES_A_PAYMENT)
[style] ~772-~772: ‘make a payment’ might be wordy. Consider a shorter alternative.
Context: ...mand requires confirmation** as it will make a payment from your account. ```sh ipb pay <acco...
(EN_WORDINESS_PREMIUM_MAKE_A_PAYMENT)
DISTRIBUTION.md
[uncategorized] ~180-~180: The official name of this software platform is spelled with a capital “H”.
Context: ...ase binaries when tags are pushed. See .github/workflows/release.yml for the automate...
(GITHUB)
[uncategorized] ~388-~388: The official name of this software platform is spelled with a capital “H”.
Context: ...or downloads ## CI/CD Integration See .github/workflows/release.yml for automated re...
(GITHUB)
🔇 Additional comments (20)
scripts/sign-macos.sh (1)
1-6: Good setup with error handling.Proper shebang and
set -eensures the script exits on unexpected errors.DISTRIBUTION.md (2)
286-287: Previous URL formatting issue appears resolved.The bare URLs flagged in prior review have been properly wrapped in angle brackets (
<https://launchpad.net>and<https://launchpad.net/people/+new-ppa>), satisfying markdownlint MD034 requirements.
1-391: Documentation is comprehensive and well-structured.The distribution guide clearly outlines multiple distribution channels, build processes, and platform-specific packaging strategies. The step-by-step instructions, code examples, and troubleshooting sections provide solid guidance for contributors and maintainers. Cross-references to related documentation (UBUNTU_DISTRIBUTION.md, CI workflow) appropriately modularize the content.
One minor note: Static analysis flagged GitHub capitalization, but these appear to be false positives (
.github/is the correct lowercase directory name). No action needed.src/cmds/run.ts (10)
1-6: LGTM!The imports properly reflect the modernized async architecture with node:fs promises, structured error handling via CliError, and the new utility functions for file operations and UI feedback.
8-18: LGTM!The extended Options interface appropriately captures all transaction simulation parameters required by the programmable card code emulator.
20-24: LGTM!Clear and concise documentation that describes the function's purpose and error handling behavior.
28-29: LGTM!Proper use of validateFilePath utility with appropriate extension restriction for JavaScript code files.
31-46: LGTM!Transaction creation and detailed logging provide clear visibility into the simulation parameters. Appropriate use of chalk for colored output.
51-61: LGTM!Proper error handling that wraps FILE_NOT_FOUND into a more specific MISSING_ENV_FILE error, providing clear feedback to users when the env file is missing.
68-74: LGTM!The try-finally block ensures the spinner is always stopped, even if file reading fails. This addresses the previous concern about terminal cleanup.
85-89: LGTM!Proper try-finally block ensures spinner cleanup on both success and error paths.
91-97: LGTM!The code execution and result logging flow is clear and provides good visibility into emulator output with appropriate color coding.
109-127: LGTM!The env parsing correctly handles:
- Values containing
=characters by using indexOf and substring- Empty values (keys with no value)
- Quoted values (strips wrapping single or double quotes)
This addresses all previously identified issues with env file parsing.
src/cmds/transfer.ts (4)
1-28: LGTM! Function signature and imports align with standardized command patterns.The updated signature using
CommonOptionsand the new utility imports properly support the enhanced interactive CLI flow. The JSDoc is comprehensive and accurately describes the parameters and behavior.
29-50: Interactive prompting with validation is well-implemented.The flow properly prompts for missing parameters and validates inputs using the standardized validation utilities. The pattern is consistent with other commands in the codebase.
Minor observation: Line 42's condition
!amountwill be true for0, which would prompt unnecessarily beforevalidateAmountrejects it. However, this is a minor edge case since users would rarely passamount=0explicitly, and the validation catches it correctly.
52-69: Confirmation flow properly implements destructive operation safeguards.The transfer summary display and confirmation prompt provide clear user feedback and align with the security-focused patterns for destructive operations. The
--yesflag integration correctly supports automation workflows while maintaining safety for interactive use.
71-99: Excellent implementation! All previous issues have been resolved.The spinner initialization, API call with retry logic, and error handling are all correctly implemented:
✅ Spinner logic fixed:
options.spinner === falsecorrectly disables the spinner
✅ Spinner started:.start()is properly called (line 72)
✅ Typo fixed: "transferring" is now spelled correctlyThe integration of
withRetryfor resilient API calls and proper spinner lifecycle management (succeed/fail) aligns with the command patterns established across the codebase.README.md (3)
60-446: Excellent documentation! Installation and configuration sections are comprehensive.The expanded documentation significantly improves the user experience:
✅ Past issue resolved: Line 275 now uses a proper Markdown heading (
#### ⚠️ Important: Secret Handling) instead of bold emphasisKey improvements:
- Multiple installation methods (npm, Homebrew, direct download) with clear instructions
- Shell autocomplete support with setup examples
- Comprehensive environment variables documentation with the new
env-listcommand- Strong security guidance emphasizing credential files over environment variables
- Thorough configuration profiles documentation with workflows and examples
The security warnings are appropriately prominent, and the examples are clear and practical.
449-874: Command documentation is clear and comprehensive.✅ Past issue resolved: Line 386 code block now includes proper language identifier (```text)
The expanded command documentation provides excellent guidance:
- Destructive operations: Clear warnings and confirmation requirements for deploy, publish, disable, transfer, and pay commands
- Automation support: Consistent
--yesflag examples for CI/CD workflows- Command examples: Comprehensive examples showing both interactive and non-interactive usage
The documentation properly reflects the enhanced CLI flows with validation, confirmations, and retry logic.
876-1196: Outstanding contributor documentation with comprehensive testing guidance.The new sections significantly improve the developer experience:
Exit Codes & Error Codes:
- Clear reference tables for automation and troubleshooting
- Practical shell script examples demonstrating error handling patterns
- Helpful error code descriptions with common scenarios and solutions
Development & Testing:
- Complete build, lint, and type-checking instructions
- Comprehensive testing documentation using Vitest
- Test structure, patterns, and coverage goals clearly documented
- Practical examples for mocking ESM modules and testing error propagation
This documentation provides everything contributors need to understand the project's quality standards and testing practices.
This pull request introduces extensive documentation and configuration improvements to the Investec Programmable Banking CLI project. The main focus is on establishing clear project guidelines, coding standards, and architectural patterns to help both new and existing contributors understand the codebase, development workflow, and best practices. No functional code changes are present; all updates are related to documentation, configuration, and project metadata.
Key documentation and configuration improvements:
Project guidelines and standards:
.cursorrulesfile detailing the technology stack, directory structure, TypeScript configuration, code patterns, error handling, command structure, utility functions, testing patterns, code quality standards, environment-specific behaviors, security considerations, command development guidelines, API integration, and common utilities reference. This serves as a central resource for contributors and enforces consistency across the project..github/copilot-instructions.mdto provide a much more detailed and structured project overview, including technology stack, project structure, architectural patterns, error handling, command structure, utility usage, configuration management, testing, coding standards, best practices, environment variables, development workflow, and common patterns. This replaces the previous brief description with a thorough contributor guide.Tooling and configuration:
.biomeignorefile to exclude thebin/directory andai-generated.jsfrom linting and formatting checks, aligning with the new Biome-based workflow and ensuring generated files are not processed by code quality tools.These changes provide a strong foundation for code quality, contributor onboarding, and maintainability by documenting all major patterns and workflows in the project.
Summary by CodeRabbit
New Features
Improvements
Distribution & CI
Tests