feat: consolidate cli tools into @cipherstash/cli#336
feat: consolidate cli tools into @cipherstash/cli#336calvinbrewer wants to merge 2 commits intomainfrom
Conversation
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR renames and consolidates the CLI into Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as Stash CLI
participant Gateway as CipherStash AI<br/>Gateway
participant Agent as Claude<br/>Agent
participant Tools as MCP<br/>Tools
participant Hooks as Security<br/>Hooks
participant DB as Project<br/>Database
User->>CLI: stash wizard
CLI->>CLI: detect project, check prerequisites, readiness
CLI->>DB: introspect schema
DB-->>CLI: tables & columns
CLI->>Gateway: fetch integration prompt
Gateway-->>CLI: prompt + version
CLI->>Agent: initialize agent (with tools, hooks)
Agent->>CLI: stream assistant messages (may request tools)
loop per tool request
Agent->>Hooks: pre-tool scan
Hooks-->>Agent: allow / block
alt allowed
Agent->>Tools: run tool (env/DB/read/write)
Tools-->>Agent: tool result
Hooks->>Hooks: post-tool content scan
else blocked
Agent->>Agent: handle block (report)
end
end
Agent->>CLI: final result (text + result message)
CLI->>CLI: format output, run post-agent steps (install/setup/push)
CLI->>User: display completion
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/cli/src/commands/secrets/helpers.ts (1)
40-42:⚠️ Potential issue | 🟠 MajorAdd
accessKeyto the missing variable validation.The
accessKeyvariable (line 37) is required and used in the returned config (line 67), but it's not validated in themissingarray check. IfCS_ACCESS_KEYis missing, users won't see it listed in the helpful error message at lines 45-55; instead, they'll encounter a generic error at line 60.🐛 Proposed fix to include accessKey validation
const missing: string[] = [] if (!workspaceCRN) missing.push('CS_WORKSPACE_CRN') if (!clientId) missing.push('CS_CLIENT_ID') if (!clientKey) missing.push('CS_CLIENT_KEY') +if (!accessKey) missing.push('CS_ACCESS_KEY')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/secrets/helpers.ts` around lines 40 - 42, The code fails to validate CS_ACCESS_KEY: add a check for accessKey to the same missing array as the other env vars so CS_ACCESS_KEY gets reported to users; specifically, in the validation block that pushes workspaceCRN, clientId, and clientKey into missing, also push 'CS_ACCESS_KEY' when accessKey is falsy so the returned config/validation in the function (where missing is used) includes accessKey.examples/basic/package.json (1)
1-23:⚠️ Potential issue | 🟠 MajorAdd required Node.js and pnpm engine constraints to the manifest.
The
examples/basic/package.jsonlacks the requiredpackageManagerandenginesfields specified in the coding guidelines for example apps. Add the following to comply withexamples/**/package.jsonrequirements:Required manifest update
{ "name": "@cipherstash/basic-example", "private": true, "version": "1.2.3", "type": "module", + "packageManager": "pnpm@9", + "engines": { + "node": ">=22", + "pnpm": "^9" + }, "scripts": { "start": "tsx index.ts" },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/basic/package.json` around lines 1 - 23, Add the required package manifest fields: add a top-level "packageManager" entry (e.g. "pnpm@8") and an "engines" object with Node and pnpm constraints (e.g. "node": ">=18" and "pnpm": ">=8") to the package.json; update the existing package.json JSON (the keys packageManager and engines) so example installs enforce the required Node and pnpm versions.
🟡 Minor comments (13)
examples/basic/index.ts-86-86 (1)
86-86:⚠️ Potential issue | 🟡 MinorAvoid logging plaintext contact data.
This logs unencrypted PII (name, email) before encryption. As per coding guidelines, example apps should not log plaintext at any time.
🛡️ Proposed fix
- console.log('Contact data to encrypt:', newContact) + console.log('Contact data prepared for encryption (fields redacted)')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/basic/index.ts` at line 86, Remove the plaintext PII logging of newContact in examples/basic/index.ts — do not call console.log('Contact data to encrypt:', newContact). Instead either remove the log entirely or replace it with a non-PII-safe message (e.g., log a generic status or masked/summary info like "Encrypting contact" or only non-sensitive metadata). Ensure the change targets the console.log that references the newContact variable so no unencrypted name/email are emitted.examples/basic/src/lib/supabase/server.ts-3-8 (1)
3-8:⚠️ Potential issue | 🟡 MinorValidate environment variables before use.
The non-null assertions (
!) will cause cryptic runtime errors ifNEXT_PUBLIC_SUPABASE_URLorNEXT_PUBLIC_SUPABASE_ANON_KEYare missing. Example apps should fail gracefully with clear guidance.🛡️ Proposed fix
export async function createServerClient() { const supabaseUrl = process.env.NEXT_PUBLIC_SUPABASE_URL! const supabaseKey = process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY! + + if (!supabaseUrl || !supabaseKey) { + throw new Error( + 'Missing Supabase environment variables. Set NEXT_PUBLIC_SUPABASE_URL and NEXT_PUBLIC_SUPABASE_ANON_KEY.', + ) + } return createClient(supabaseUrl, supabaseKey) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/basic/src/lib/supabase/server.ts` around lines 3 - 8, The createServerClient function currently uses non-null assertions on NEXT_PUBLIC_SUPABASE_URL and NEXT_PUBLIC_SUPABASE_ANON_KEY which will throw unclear runtime errors if missing; update createServerClient to explicitly read process.env.NEXT_PUBLIC_SUPABASE_URL and process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY, validate they are defined and non-empty, and if not throw a clear, actionable Error (e.g., "Missing NEXT_PUBLIC_SUPABASE_URL" / "Missing NEXT_PUBLIC_SUPABASE_ANON_KEY") before calling createClient so example apps fail fast with a helpful message.packages/cli/src/commands/db/setup.ts-124-124 (1)
124-124:⚠️ Potential issue | 🟡 MinorInconsistent branding in outro message.
The outro still references "CipherStash Forge" while the rest of the file uses the new
stash dbcommand naming. This also applies to line 143.🧹 Proposed fix
- p.outro('CipherStash Forge setup complete!') + p.outro('stash db setup complete!')And similarly at line 143:
- p.outro('CipherStash Forge setup complete!') + p.outro('stash db setup complete!')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/db/setup.ts` at line 124, Replace the legacy "CipherStash Forge" branding in the CLI outro messages with the new `stash db` naming by updating the two p.outro(...) calls that currently contain "CipherStash Forge setup complete!" (and the second similar message later in the file) so they read something like "stash db setup complete!" to match the rest of the command naming; locate the p.outro usages in this file and update their string literals accordingly.examples/basic/src/lib/supabase/encrypted.ts-2-2 (1)
2-2:⚠️ Potential issue | 🟡 MinorRemove unused import:
contactsTable.
contactsTableis imported on line 2 but never used in the file. Remove it from the import statement.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/basic/src/lib/supabase/encrypted.ts` at line 2, The import includes an unused symbol contactsTable; update the import statement to only import encryptionClient (remove contactsTable) from '../../encryption/index' so the file no longer imports an unused identifier.packages/cli/src/bin/stash.ts-198-200 (1)
198-200:⚠️ Potential issue | 🟡 Minor
--helpnever reaches command-specific handlers.Because the global
flags.helpbranch returns before dispatch,stash auth --helpprints the top-level help instead of the dedicated auth help text. The same pattern will block any future command-specific--helphandling too.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/bin/stash.ts` around lines 198 - 200, The global help check currently returns early when flags.help is set, preventing command-specific handlers from seeing `--help` (so `command`, `flags.help`, and `HELP` are involved). Modify the condition so that flags.help only triggers the top-level help when no command is provided (e.g., keep printing HELP if !command or command is the top-level `--help`/`-h`, but do not return just because flags.help is true when a specific command exists). Update the if in stash.ts to only treat flags.help as top-level help when command is falsy (or move the flags.help check after dispatch), ensuring command-specific handlers still receive `--help`.packages/cli/README.md-89-89 (1)
89-89:⚠️ Potential issue | 🟡 MinorFix markdownlint MD058 table spacing warnings.
Several tables are missing required blank lines around them. This is currently flagged by markdownlint and should be normalized to keep docs lint-clean.
Also applies to: 135-135, 167-167, 187-187, 193-193, 218-218
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/README.md` at line 89, Several Markdown tables in the README are missing the required blank lines before and after them (causing markdownlint MD058 warnings); edit the README.md to ensure each table has an empty line immediately above and below the table block (i.e., add a blank line before the table header row and another blank line after the last table row) for every occurrence flagged (the tables mentioned in the review), then re-run markdownlint to confirm the MD058 warnings are resolved.packages/cli/src/commands/wizard/__tests__/agent-sdk.test.ts-1-5 (1)
1-5:⚠️ Potential issue | 🟡 MinorAdd
dotenv/configimport for env-dependent integration tests.This test reads environment variables but does not import dotenv bootstrap at the top.
As per coding guidelines, "Import dotenv/config at the top of test files that need environment variables".Suggested patch
+import 'dotenv/config' import { describe, it, expect, beforeAll } from 'vitest'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/__tests__/agent-sdk.test.ts` around lines 1 - 5, The test file agent-sdk.test.ts uses environment variables but doesn't bootstrap dotenv; add an import 'dotenv/config' as the very first import in packages/cli/src/commands/wizard/__tests__/agent-sdk.test.ts (before describe, before any other imports) so env-dependent integration tests get loaded; keep the rest of the file intact.packages/cli/src/commands/wizard/run.ts-91-94 (1)
91-94:⚠️ Potential issue | 🟡 MinorMissing
shutdownAnalytics()on cancel paths.When the user cancels at lines 91-94 or 206-209,
shutdownAnalytics()is not called. While cancellation is a clean exit, any events captured before cancellation (e.g.,trackFrameworkDetected) won't be flushed.🛡️ Proposed fix for cancellation handling
if (p.isCancel(confirmed)) { p.cancel('Cancelled.') + await shutdownAnalytics() process.exit(0) }And in
selectIntegration:if (p.isCancel(selected)) { p.cancel('Cancelled.') + await shutdownAnalytics() process.exit(0) }Note:
selectIntegrationwould need to beasyncand the shutdown call would need to be handled, or move the cancel check to the caller.Also applies to: 206-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/run.ts` around lines 91 - 94, The cancel branches that call p.cancel('Cancelled.') then process.exit(0) are missing a call to shutdownAnalytics(), so update both cancel paths (the p.isCancel checks in runWizard or run command and the similar check in selectIntegration) to call await shutdownAnalytics() before exiting; if selectIntegration is currently synchronous, make selectIntegration async (or return a cancel result to the caller) so you can await shutdownAnalytics() there or let the caller perform the shutdown and exit, ensuring shutdownAnalytics() runs and flushes events (e.g., after p.cancel and before process.exit).packages/cli/src/commands/wizard/agent/fetch-prompt.ts-69-76 (1)
69-76:⚠️ Potential issue | 🟡 MinorWrap successful response JSON parsing in try/catch.
If the gateway returns a 200 OK with malformed JSON,
res.json()will throw an unhandled exception. This should be caught and formatted consistently with other error paths.🛡️ Proposed fix
- const body = (await res.json()) as Partial<FetchedPrompt> + let body: Partial<FetchedPrompt> + try { + body = (await res.json()) as Partial<FetchedPrompt> + } catch { + throw new Error( + formatWizardError( + 'The wizard gateway returned an invalid response.', + 'Could not parse JSON body.', + ), + ) + } if (typeof body.prompt !== 'string' || typeof body.promptVersion !== 'string') {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/agent/fetch-prompt.ts` around lines 69 - 76, Wrap the await res.json() call in a try/catch so malformed JSON from a 200 response is handled; specifically, around the expression that assigns body = (await res.json()) as Partial<FetchedPrompt> catch JSON parse errors and throw a new Error(formatWizardError(...)) consistent with existing error paths (use the same message 'The wizard gateway returned an invalid prompt response.' or augment with the caught error.message). Ensure you reference the same symbols: res.json(), body, and formatWizardError when making the change.packages/cli/src/commands/schema/build.ts-426-426 (1)
426-426:⚠️ Potential issue | 🟡 Minor
config.databaseUrlmay be undefined.If
loadStashConfig()returns a config withoutdatabaseUrl, passingundefinedtointrospectDatabasewill causepg.Clientto fail with an unclear error. Add validation before proceeding.🛡️ Proposed fix
+ if (!config.databaseUrl) { + p.log.error('DATABASE_URL not configured. Run `stash db setup` first.') + p.cancel('Missing database configuration.') + return + } + const schemas = await buildSchemasFromDatabase(config.databaseUrl)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/schema/build.ts` at line 426, Validate that config.databaseUrl is defined before calling buildSchemasFromDatabase; if loadStashConfig() returned a config without databaseUrl, throw or log a clear error and exit rather than passing undefined into buildSchemasFromDatabase/introspectDatabase (which constructs pg.Client). Add a guard in the code path where buildSchemasFromDatabase(config.databaseUrl) is invoked to check the value, and surface a helpful message referencing the missing databaseUrl so callers can fix the config.packages/cli/src/commands/wizard/lib/post-agent.ts-27-33 (1)
27-33:⚠️ Potential issue | 🟡 MinorRemove the guard check for
installCommand— it is always defined.The
GatheredContext.installCommandis guaranteed to be a non-empty string. Ingather.ts(lines 47–49), it is assigned via:packageManager.installCommand + '@cipherstash/stack'if detected, or defaults to'npm install@cipherstash/stack'otherwise. No code path allows it to be undefined or empty.However, lines 112–113 in
post-agent.tslog plaintext messages, which violates the coding guideline that prohibits plaintext logging:p.log.warn(`Command failed: ${message}`) p.log.info(`You can run this manually: ${command}`)Remove or refactor these log statements to avoid exposing command text and error details in plaintext.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/lib/post-agent.ts` around lines 27 - 33, Remove the unnecessary guard around GatheredContext.installCommand (it's always a non-empty string) and always call runStep('Installing `@cipherstash/stack`...', 'Package installed', gathered.installCommand, cwd) as currently written; also eliminate the plaintext-exposing logs p.log.warn(`Command failed: ${message}`) and p.log.info(`You can run this manually: ${command}`) in post-agent.ts — replace them with non-sensitive, guideline-compliant messages (e.g., p.log.warn('Installation command failed; see logs for details') and p.log.info('If needed, re-run the installation with your package manager')) or mask the command/error details while preserving context, referencing the identifiers gathered.installCommand, runStep, p.log.warn, p.log.info, and the local variables message/command to locate and update the code.packages/cli/src/commands/wizard/__tests__/wizard-tools.test.ts-174-185 (1)
174-185:⚠️ Potential issue | 🟡 MinorStale comment in test.
Line 181 mentions "Actually, we just wrote 'DANGER.*=other' above" but the test writes
NORMAL_KEY=value. This appears to be a copy-paste artifact. The test logic is correct — it verifies.*is not matched when no literal.*key exists.📝 Proposed fix
it('escapes metacharacters so they match literally', () => { writeFileSync(join(tmp, '.env'), 'NORMAL_KEY=value\n') const result = checkEnvKeys(tmp, { filePath: '.env', keys: ['.*'], // Should NOT match NORMAL_KEY }) - // ".*" is not literally in the file as a key - // Actually, we just wrote "DANGER.*=other" above, different test - // Here, ".*" should be missing because there's no literal ".*" key + // ".*" is not literally in the file as a key, so it should be reported as missing expect(result['.*']).toBe('missing') })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/__tests__/wizard-tools.test.ts` around lines 174 - 185, The inline comment inside the test "escapes metacharacters so they match literally" is stale/misleading (it references "DANGER.*=other" even though the test writes NORMAL_KEY=value); update or remove that sentence so the comment accurately reflects the test using checkEnvKeys(...) with keys ['.*'] and expecting result['.*'] === 'missing' (or reword to state there's no literal ".*" key), ensuring clarity around the test name and the use of checkEnvKeys and the literal key '.*'.packages/cli/src/commands/wizard/tools/wizard-tools.ts-107-116 (1)
107-116:⚠️ Potential issue | 🟡 Minor
ensureGitignoresilently skips if.gitignoredoesn't exist.If the project doesn't have a
.gitignorefile,ensureGitignorereturns early without creating one or warning the user. This could lead to accidentally committing sensitive.envfiles.🛡️ Proposed fix to create .gitignore if missing
function ensureGitignore(cwd: string, envFile: string) { const gitignorePath = resolve(cwd, '.gitignore') - if (!existsSync(gitignorePath)) return + if (!existsSync(gitignorePath)) { + writeFileSync(gitignorePath, `${envFile}\n`, 'utf-8') + return + } const content = readFileSync(gitignorePath, 'utf-8') if (!content.includes(envFile)) { appendFileSync(gitignorePath, `\n${envFile}\n`) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/tools/wizard-tools.ts` around lines 107 - 116, ensureGitignore currently returns early when .gitignore is missing, which risks committing env files; update ensureGitignore to create .gitignore when it doesn't exist (using gitignorePath) and write the envFile entry, and when it does exist readFileSync and append envFile only if not present (preserve newline formatting), ensuring envFile (the passed envFile arg) is added exactly once.
🧹 Nitpick comments (21)
packages/cli/src/commands/secrets/helpers.ts (1)
59-61: Consider removing redundant validation check.Once the
accessKeyvalidation is added to themissingarray check (lines 40-42), this subsequent check becomes unreachable. If any required variable is missing,process.exit(1)is called at line 56, so line 59 can never encounter missing values.♻️ Proposed cleanup to remove redundant check
- if (!workspaceCRN || !clientId || !clientKey || !accessKey) { - throw new Error('Missing required configuration') - } - return {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/secrets/helpers.ts` around lines 59 - 61, The if-check that throws new Error('Missing required configuration') is redundant because the same required vars (workspaceCRN, clientId, clientKey, accessKey) are already validated via the missing array and cause process.exit(1) earlier; remove the entire if block referencing workspaceCRN, clientId, clientKey, accessKey in helpers.ts to avoid unreachable code and rely on the missing-array validation logic instead.packages/cli/src/commands/init/steps/build-schema.ts (1)
55-64: Consider adding error handling for file write failures.The
writeFileSynccall (line 61) can throw on permission errors or disk space issues. While the error will propagate, a user-friendly message would improve the experience.♻️ Suggested enhancement
// Write the file const dir = dirname(resolvedPath) if (!existsSync(dir)) { mkdirSync(dir, { recursive: true }) } - writeFileSync(resolvedPath, fileContents, 'utf-8') - p.log.success(`Encryption client written to ${clientFilePath}`) + try { + writeFileSync(resolvedPath, fileContents, 'utf-8') + p.log.success(`Encryption client written to ${clientFilePath}`) + } catch (err) { + p.log.error(`Failed to write encryption client: ${err instanceof Error ? err.message : String(err)}`) + return { ...state, schemaGenerated: false } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/init/steps/build-schema.ts` around lines 55 - 64, The writeFileSync call using resolvedPath and fileContents can throw and should be wrapped in a try/catch: catch filesystem errors around the block that creates dir (dirname/existsSync/mkdirSync) and calls writeFileSync, call p.log.error with a clear user-facing message including the error details, and then either rethrow or return a failure state instead of letting an uncaught exception bubble; update the return to only execute p.log.success and return { ...state, clientFilePath, schemaGenerated: true } when the write succeeds.packages/cli/src/commands/wizard/__tests__/detect.test.ts (1)
109-157: Consider adding a test for lockfile precedence.The tests cover individual lockfile detection, but there's no test for when multiple lockfiles exist (e.g., both
yarn.lockandpackage-lock.json). This could help document the expected precedence behavior.💡 Optional test case
+ it('prefers bun over other lockfiles when multiple exist', () => { + writeFileSync(join(tmp, 'bun.lock'), '') + writeFileSync(join(tmp, 'package-lock.json'), '') + const pm = detectPackageManager(tmp) + expect(pm?.name).toBe('bun') + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/__tests__/detect.test.ts` around lines 109 - 157, Add a new unit test in detectPackageManager's test suite that creates multiple lockfiles in the same temp dir to assert the precedence behavior (e.g., write both yarn.lock and package-lock.json and assert detectPackageManager(tmp).name is the expected winner). Locate the tests around the describe('detectPackageManager') block in detect.test.ts and add a case named like 'resolves precedence when multiple lockfiles exist' that writes multiple lockfiles, calls detectPackageManager(tmp), and asserts the chosen package manager and its installCommand to document and lock in the intended precedence.packages/cli/src/commands/wizard/lib/format.ts (1)
54-59: Minor: The checkmark regex may not match common variants.The regex
/^\s*[-*]?\s*✅/specifically looks for the ✅ emoji, but the docstring at line 54 also mentions✓and✔. Consider expanding the pattern to match these variants if the agent might output them.♻️ Suggested enhancement
- // Checkmark lines: ✅ or - ✅ or * ✅ - if (/^\s*[-*]?\s*✅/.test(line)) { - const content = line.replace(/^\s*[-*]?\s*✅\s*/, '') + // Checkmark lines: ✅ or ✓ or ✔ (with optional bullet prefix) + if (/^\s*[-*]?\s*[✅✓✔]/.test(line)) { + const content = line.replace(/^\s*[-*]?\s*[✅✓✔]\s*/, '')🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/lib/format.ts` around lines 54 - 59, The checkmark detection currently only matches the ✅ emoji using the regex in the block that tests `line`; update the pattern to also accept the other common variants (✓ and ✔) and keep handling optional leading bullets and whitespace; specifically change the test regex to use a character class like `[✅✓✔]` (or equivalent) and update the corresponding `replace` call used to compute `content` so it strips any of those characters as well (the surrounding symbols: the `line` variable, the test regex, the `replace` call, `formatInline` and the `pc.green('✔')` call should remain as-is aside from the regex change).packages/cli/src/commands/wizard/__tests__/gateway-messages.test.ts (1)
221-242: Overly permissive assertion may mask gateway errors.The assertion
expect(res.status).toBeGreaterThanOrEqual(200)passes for any status including 5xx server errors. While the comment acknowledges this is for dev gateway compatibility, consider tightening to explicitly allow expected statuses:- // Just verify we get a response (not a crash). - expect(res.status).toBeGreaterThanOrEqual(200) + // Dev gateway may pass through; production should return 401. + // Accept 200-499 range (client/auth errors) but fail on 5xx server errors. + expect(res.status).toBeLessThan(500)This still allows flexibility while catching unexpected server crashes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/__tests__/gateway-messages.test.ts` around lines 221 - 242, The test "rejects invalid auth token with non-200 status" currently uses a too-permissive assertion on res.status; update the assertion on the response (variable res) to explicitly allow only the expected statuses (e.g., 200 for dev pass-through, 401 for production auth rejection, and optionally 403) instead of using expect(res.status).toBeGreaterThanOrEqual(200); for example, replace that line with an explicit containment check that res.status is one of [200, 401, 403] so unexpected 5xx errors will fail the test.examples/basic/index.ts (1)
4-4: Unused imports.
getAllContactsandcreateContactare imported but never called (the usage at lines 90-91 is commented out). Either remove these imports or uncomment the demonstration code.🧹 Proposed fix
-import { getAllContacts, createContact } from './src/queries/contacts'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/basic/index.ts` at line 4, The import of getAllContacts and createContact is unused; either remove those imports from the top of the file (delete getAllContacts and createContact from the import statement) or restore the demo usage by uncommenting the example calls that reference getAllContacts() and createContact(...) so the imports are actually used; update the import statement and/or uncomment the demonstration code accordingly to eliminate the unused-import warning.examples/basic/src/lib/supabase/encrypted.ts (1)
5-5: Top-level await limits CJS compatibility.Top-level
awaitis an ESM-only feature. If this module needs to be consumed viarequire(), it will fail. As per coding guidelines, example apps should keep both ESM and CJS exports working.Consider wrapping in an async initializer if CJS support is required:
♻️ Alternative pattern for broader compatibility
-const supabase = await createServerClient() -export const eSupabase = encryptedSupabase({ - encryptionClient, - supabaseClient: supabase, -}) +let _eSupabase: ReturnType<typeof encryptedSupabase> | null = null + +export async function getEncryptedSupabase() { + if (!_eSupabase) { + const supabase = await createServerClient() + _eSupabase = encryptedSupabase({ + encryptionClient, + supabaseClient: supabase, + }) + } + return _eSupabase +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/basic/src/lib/supabase/encrypted.ts` at line 5, Top-level await using createServerClient() breaks CommonJS consumers; change to an async initializer pattern: remove the top-level await and instead export a function (e.g., initSupabase or getSupabaseClient) that constructs and returns the supabase client via await createServerClient(), or implement a lazy-initializing getter that calls an internal async initializer the first time; update any imports/usage in examples to call the async initializer rather than relying on top-level await.packages/cli/src/commands/wizard/lib/analytics.ts (2)
28-29: Move imports to the top of the file.The
createHash,hostname, anduserInfoimports on lines 28-29 are placed in the middle of the file after thegetClientfunction. Moving them to the top with other imports improves readability and follows standard conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/lib/analytics.ts` around lines 28 - 29, Move the imports createHash, hostname, and userInfo to the top of the module with the other imports to follow convention and improve readability; locate the current middle-file import statements (after the getClient function) and relocate them to the file header so the symbols createHash, hostname, and userInfo are imported before any function definitions (including getClient) that may reference them.
106-115: Consider sanitizing error messages before tracking.The
errorparameter is sent directly to PostHog. If error messages ever contain tokens, connection strings, or other sensitive data (e.g., from database connection errors or auth failures), this could leak PII to analytics.Consider truncating or sanitizing the error string before capture.
♻️ Proposed sanitization
+function sanitizeError(error: string): string { + // Truncate and redact potential secrets + return error + .replace(/Bearer\s+\S+/gi, 'Bearer [REDACTED]') + .replace(/postgres(ql)?:\/\/[^\s]+/gi, '[DATABASE_URL]') + .slice(0, 500) +} + export function trackWizardError(error: string, integration?: Integration) { getClient()?.capture({ distinctId: getDistinctId(), event: 'wizard error', properties: { - error, + error: sanitizeError(error), integration: integration ?? 'unknown', }, }) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/lib/analytics.ts` around lines 106 - 115, trackWizardError currently sends raw error strings to PostHog which can leak sensitive data; before calling getClient()?.capture, sanitize the error parameter in trackWizardError by stripping/masking likely secrets (e.g., tokens, connection strings, credentials, emails) and truncating to a safe length (e.g., 200 chars), then send the sanitized version in the properties while preserving integration and distinctId via getDistinctId(); implement the sanitizer as a helper used by trackWizardError so other callers can reuse it.packages/cli/src/commands/wizard/run.ts (1)
179-212:selectIntegrationcallsprocess.exitdirectly, bypassing caller control.The helper function exits the process on cancel, preventing the caller from performing cleanup. Consider returning
undefinedon cancel and lettingrun()handle the exit with proper analytics shutdown.♻️ Proposed refactor
-async function selectIntegration(): Promise<Integration> { +async function selectIntegration(): Promise<Integration | undefined> { const selected = await p.select<Integration>({ message: 'Which integration are you using?', options: [ // ... ], }) if (p.isCancel(selected)) { - p.cancel('Cancelled.') - process.exit(0) + return undefined } return selected }Then in the caller:
} else { - selectedIntegration = await selectIntegration() + const sel = await selectIntegration() + if (!sel) { + p.cancel('Cancelled.') + await shutdownAnalytics() + process.exit(0) + } + selectedIntegration = sel }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/run.ts` around lines 179 - 212, selectIntegration currently calls process.exit on cancel which prevents callers from doing cleanup; change selectIntegration to return Promise<Integration | undefined>, remove the direct process.exit call (but you can still call p.cancel('Cancelled.') if desired) and return undefined when p.isCancel(selected) is true; update the caller run() to check for an undefined result from selectIntegration, perform any necessary shutdown/analytics cleanup, and then exit the process or return as appropriate.packages/cli/src/commands/schema/build.ts (1)
174-183: Generated code uses top-levelawait.The generated encryption client uses
await Encryption({...})at module scope (lines 180 and 218). This requires:
- ES modules (
"type": "module"in package.json or.mjsextension)- Node.js 14.8+ (or bundler support)
Consider adding a comment in the generated code or warning users if their project doesn't appear to support ESM.
Also applies to: 213-221
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/schema/build.ts` around lines 174 - 183, The generated module exports use top-level await when creating encryptionClient via Encryption({...}), which can break projects not using ESM or older Node versions; update the code generator (the template that produces encryptionClient and the call to Encryption) to either (a) avoid top-level await by exporting an async init function (e.g., export async function initEncryptionClient() { return await Encryption(...) }) and document how to call it, or (b) inject a clear comment at the top of the generated file warning that top-level await/ESM is required (mentioning Encryption and encryptionClient by name), and surface a CLI warning when the user's project package type or Node version suggests CJS/older Node so they can opt into ESM or use the init function.packages/cli/src/commands/wizard/lib/post-agent.ts (1)
94-115: Error swallowing may silently mask critical failures.The
runStephelper catches errors and continues to the next step. While this provides resilience, critical steps likestash db setuporstash db pushfailing could leave the system in an inconsistent state. The user sees "You can run this manually" but may not realize subsequent steps depend on the failed one.Consider either:
- Returning a success boolean so callers can decide whether to proceed
- Distinguishing between recoverable and critical steps
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/lib/post-agent.ts` around lines 94 - 115, The runStep helper currently swallows errors inside runStep, which can hide failures of critical commands like "stash db setup" or "stash db push"; change runStep to return a boolean (or throw on critical failures) so callers can decide to abort further steps: update the signature of runStep to Promise<boolean>, return true on success and false on catch (or accept an optional parameter like isCritical and rethrow when isCritical is true), keep the existing logging (include the command and error message), and update callers that invoke runStep to check the boolean result and stop or handle failures for critical steps (e.g., stall further steps when stash db setup or stash db push returns false).packages/cli/src/commands/wizard/lib/gather.ts (2)
169-172: Sameprocess.exit()concern in selection functions.Multiple
process.exit(0)calls on user cancellation. These could throw aCancelledErrorinstead for consistency with modern CLI patterns and better testability.Also applies to: 204-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/lib/gather.ts` around lines 169 - 172, The cancellation branch in the selection handlers (the p.isCancel(selectedTables) checks that call p.cancel('Cancelled.') then process.exit(0)) should not call process.exit; instead throw a CancelledError to propagate cancellation consistently and improve testability. Replace the process.exit(0) calls in the blocks handling selectedTables (and the similar block at lines 204-207) with throwing new CancelledError('Cancelled.') or rethrowing an existing CancelledError type used by the CLI, keeping the p.cancel('Cancelled.') call if desired for prompt cleanup before throwing.
66-70:process.exit()makes the function difficult to test and reuse.Direct
process.exit(0)calls prevent proper testing and reuse ofgatherContext. Consider throwing a specific error or returning early to let the caller handle the exit.♻️ Proposed alternative
if (selectedColumns.length === 0) { p.log.warn('No columns selected for encryption.') p.cancel('Nothing to do.') - process.exit(0) + throw new Error('No columns selected for encryption') }Then handle this in the caller (e.g.,
run.ts) with appropriate exit logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/lib/gather.ts` around lines 66 - 70, In gatherContext replace the direct process.exit(0) call inside the selectedColumns.length === 0 branch with non-exiting control flow: either throw a specific, named error (e.g., NoColumnsSelectedError) or return a clear sentinel/result so the caller can decide to exit; update the branch that currently calls p.log.warn('No columns selected for encryption.') and p.cancel('Nothing to do.') to then throw that error or return, and update the caller (e.g., run.ts) to catch that error / check the sentinel and call process.exit(0) there so tests can handle gatherContext without the process terminating.packages/cli/src/commands/wizard/tools/wizard-tools.ts (3)
120-120: Move import to top of file.The import on line 120 should be grouped with other imports at the top of the file for consistency and readability.
♻️ Proposed fix
Move the import to the top:
import { existsSync, readFileSync, writeFileSync, appendFileSync } from 'node:fs' import { resolve, relative } from 'node:path' import pg from 'pg' +import { detectPackageManager as detect } from '../lib/detect.js'Then remove line 120.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/tools/wizard-tools.ts` at line 120, The inline import "import { detectPackageManager as detect } from '../lib/detect.js'" should be moved up into the existing import block at the top of wizard-tools.ts and grouped with the other imports for consistency; add that import alongside the other top-level imports (keeping the alias "detect") and then remove the duplicate inline import statement currently at line 120 to avoid duplicate imports and maintain readability.
150-191: Consider adding a connection timeout.The database connection has no timeout configured. If the database is unreachable or slow, the operation could hang indefinitely.
♻️ Proposed fix
export async function introspectDatabase( databaseUrl: string, ): Promise<DbTable[]> { - const client = new pg.Client({ connectionString: databaseUrl }) + const client = new pg.Client({ + connectionString: databaseUrl, + connectionTimeoutMillis: 10000, // 10 second timeout + }) try { await client.connect()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/tools/wizard-tools.ts` around lines 150 - 191, The introspectDatabase function currently creates a pg.Client without any timeout so client.connect()/client.query() can hang; update the new pg.Client instantiation in introspectDatabase to include a connection timeout (e.g., connectionTimeoutMillis: 5000) and after client.connect() set a per-query/server timeout (e.g., run await client.query("SET statement_timeout = 5000") or use client.query with a timeout option if your pg version supports it) so both the connection and the metadata query will fail fast instead of hanging; adjust the numeric timeout value to your desired limit and ensure you still call client.end() in the finally block.
26-32: Simplify path traversal check for clarity.Line 29's condition
resolve(resolved) !== resolved.replace(/\/$/, '')is redundant and unclear. Sinceresolvedis already normalized byresolve(), callingresolve()again on it produces the same value, and the trailing-slash check doesn't strengthen the security validation.Replace with
rel.startsWith('/'), which correctly blocks absolute paths that escape the cwd while being more straightforward:♻️ Proposed simplification
function assertWithinCwd(cwd: string, filePath: string): void { const resolved = resolve(cwd, filePath) const rel = relative(cwd, resolved) - if (rel.startsWith('..') || resolve(resolved) !== resolved.replace(/\/$/, '')) { + if (rel.startsWith('..') || rel.startsWith('/')) { throw new Error(`Path traversal blocked: ${filePath} resolves outside the project directory.`) } }The
rel.startsWith('/')check handles absolute paths that escape the cwd. Both approaches are functionally equivalent across all path traversal scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/tools/wizard-tools.ts` around lines 26 - 32, The path-traversal check in assertWithinCwd is using a redundant and unclear second condition (resolve(resolved) !== resolved.replace(/\/$/, '')); remove that redundancy and replace the if condition to check only for rel.startsWith('..') or rel.startsWith('/'), i.e. update the check that uses cwd, filePath, resolved and rel so that it throws when rel.startsWith('..') || rel.startsWith('/'), eliminating the extra resolve/trim logic.packages/cli/src/commands/wizard/agent/hooks.ts (2)
58-76: Consider expanding secret detection patterns.The current patterns cover PostHog, Stripe live keys, and basic hardcoded passwords. Consider adding patterns for other common secrets (AWS keys, GitHub tokens, generic API keys like
api_key=,apikey=). This can be iteratively improved.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/agent/hooks.ts` around lines 58 - 76, The secret scanner (SECRET_PATTERNS used by scanPostToolUseWrite) is too narrow; extend it to catch other common secrets by adding regex entries for AWS access keys (AKIA[0-9A-Z]{16}), AWS secret patterns (secret|aws_secret|aws_secret_access_key), GitHub tokens (ghp_[A-Za-z0-9_/-]{36} and github_token variants), generic API keys (api_key|apikey\s*=\s*['"][^'"]+['"]), and other providers (e.g., slack, sendgrid, firebase tokens) with appropriate rule and reason labels; update SECRET_PATTERNS array only and keep scanPostToolUseWrite logic the same so any matched pattern returns {blocked: true, rule, reason}.
8-12: ExportScanResultinterface for consumers.The
ScanResultinterface is used as the return type for all exported scanner functions but is not itself exported. Consumers of these functions (e.g.,interface.ts) may need to type-check results.♻️ Proposed fix
-interface ScanResult { +export interface ScanResult { blocked: boolean rule?: string reason?: string }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/agent/hooks.ts` around lines 8 - 12, The ScanResult interface is currently only declared locally and needs to be exported so consumers can type-check scanner results; update the declaration of ScanResult (the interface named ScanResult in this file) to be exported (export interface ScanResult { ... }) and ensure any files that import or reference it (for example interface.ts and any exported scanner functions that return ScanResult) import the exported type so the return types align with the public API.packages/cli/src/commands/wizard/agent/interface.ts (1)
338-341: Debug logging may expose sensitive data.Agent stderr output could potentially contain sensitive information. While this is gated behind
session.debug, consider sanitizing or truncating the output similarly to how result messages are truncated at line 472-473.♻️ Proposed fix
stderr: session.debug - ? (data: string) => { p.log.warn(`[agent stderr] ${data.trim()}`) } + ? (data: string) => { p.log.warn(`[agent stderr] ${data.trim().slice(0, 500)}`) } : undefined,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/agent/interface.ts` around lines 338 - 341, The stderr callback currently logs raw stderr when session.debug is true via (data: string) => { p.log.warn(`[agent stderr] ${data.trim()}`) }; update this to sanitize and truncate the data before logging (reuse the same truncation/sanitization utility used for result messages) so you call that helper on data.trim() and then p.log.warn the sanitized/truncated string; ensure the sanitizer removes or masks obvious secrets (e.g., tokens, bearer headers, emails) and enforces a max length consistent with the existing result message truncation.packages/cli/src/commands/wizard/__tests__/wizard-tools.test.ts (1)
187-213: Consider adding tests for other package managers.The test only covers pnpm detection. Consider adding similar tests for npm (
package-lock.json), yarn (yarn.lock), and bun (bun.lockb) to ensure complete coverage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/cli/src/commands/wizard/__tests__/wizard-tools.test.ts` around lines 187 - 213, Add unit tests for detectPackageManagerTool to cover npm, yarn, and bun detection similar to the existing pnpm test: create temp dir, write the corresponding lockfile (package-lock.json for npm, yarn.lock for yarn, bun.lockb for bun), call detectPackageManagerTool(tmp), and assert result.detected is true and that the returned name, installCommand, and runCommand match the expected values (e.g., npm => name: 'npm', installCommand: 'npm install' or 'npm i' per project convention, runCommand: 'npm run'; yarn => 'yarn', 'yarn add', 'yarn run'; bun => 'bun', 'bun add', 'bun run'). Use the same beforeEach/afterEach tmp setup and mirror the it(...) structure used for the pnpm test so tests are consistent and isolated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 132a0a0f-4a57-4699-9b4e-47cb79c9ab3c
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (88)
examples/basic/index.tsexamples/basic/package.jsonexamples/basic/src/encryption/index.tsexamples/basic/src/lib/supabase/encrypted.tsexamples/basic/src/lib/supabase/server.tsexamples/basic/src/queries/contacts.tsexamples/basic/stash.config.tspackages/cli/CHANGELOG.mdpackages/cli/README.mdpackages/cli/package.jsonpackages/cli/src/__tests__/config.test.tspackages/cli/src/__tests__/installer.test.tspackages/cli/src/bin/stash.tspackages/cli/src/commands/auth/index.tspackages/cli/src/commands/auth/login.tspackages/cli/src/commands/db/install.tspackages/cli/src/commands/db/push.tspackages/cli/src/commands/db/setup.tspackages/cli/src/commands/db/status.tspackages/cli/src/commands/db/test-connection.tspackages/cli/src/commands/db/upgrade.tspackages/cli/src/commands/db/validate.tspackages/cli/src/commands/index.tspackages/cli/src/commands/init/index.tspackages/cli/src/commands/init/providers/base.tspackages/cli/src/commands/init/providers/drizzle.tspackages/cli/src/commands/init/providers/supabase.tspackages/cli/src/commands/init/steps/authenticate.tspackages/cli/src/commands/init/steps/build-schema.tspackages/cli/src/commands/init/steps/install-forge.tspackages/cli/src/commands/init/steps/next-steps.tspackages/cli/src/commands/init/steps/select-connection.tspackages/cli/src/commands/init/types.tspackages/cli/src/commands/init/utils.tspackages/cli/src/commands/schema/build.tspackages/cli/src/commands/secrets/delete.tspackages/cli/src/commands/secrets/get-many.tspackages/cli/src/commands/secrets/get.tspackages/cli/src/commands/secrets/helpers.tspackages/cli/src/commands/secrets/index.tspackages/cli/src/commands/secrets/list.tspackages/cli/src/commands/secrets/set.tspackages/cli/src/commands/wizard/__tests__/agent-sdk.test.tspackages/cli/src/commands/wizard/__tests__/commandments.test.tspackages/cli/src/commands/wizard/__tests__/detect.test.tspackages/cli/src/commands/wizard/__tests__/format.test.tspackages/cli/src/commands/wizard/__tests__/gateway-messages.test.tspackages/cli/src/commands/wizard/__tests__/health-checks.test.tspackages/cli/src/commands/wizard/__tests__/hooks.test.tspackages/cli/src/commands/wizard/__tests__/interface.test.tspackages/cli/src/commands/wizard/__tests__/wizard-tools.test.tspackages/cli/src/commands/wizard/agent/commandments.tspackages/cli/src/commands/wizard/agent/errors.tspackages/cli/src/commands/wizard/agent/fetch-prompt.tspackages/cli/src/commands/wizard/agent/hooks.tspackages/cli/src/commands/wizard/agent/interface.tspackages/cli/src/commands/wizard/health-checks/index.tspackages/cli/src/commands/wizard/lib/analytics.tspackages/cli/src/commands/wizard/lib/constants.tspackages/cli/src/commands/wizard/lib/detect.tspackages/cli/src/commands/wizard/lib/format.tspackages/cli/src/commands/wizard/lib/gather.tspackages/cli/src/commands/wizard/lib/post-agent.tspackages/cli/src/commands/wizard/lib/prerequisites.tspackages/cli/src/commands/wizard/lib/types.tspackages/cli/src/commands/wizard/run.tspackages/cli/src/commands/wizard/tools/wizard-tools.tspackages/cli/src/config/index.tspackages/cli/src/index.tspackages/cli/src/installer/index.tspackages/cli/src/sql/cipherstash-encrypt-no-operator-family.sqlpackages/cli/src/sql/cipherstash-encrypt-supabase.sqlpackages/cli/src/sql/cipherstash-encrypt.sqlpackages/cli/tsconfig.jsonpackages/cli/tsup.config.tspackages/cli/vitest.config.tspackages/stack-forge/README.mdpackages/stack-forge/src/bin/stash-forge.tspackages/stack-forge/src/commands/index.tspackages/stack/README.mdpackages/stack/package.jsonpackages/stack/src/bin/commands/auth/index.tspackages/stack/src/bin/commands/init/steps/authenticate.tspackages/stack/src/bin/commands/init/steps/build-schema.tspackages/stack/src/bin/stash.tspackages/stack/tsup.config.tspnpm-workspace.yamlskills/stash-cli/SKILL.md
💤 Files with no reviewable changes (8)
- packages/stack/tsup.config.ts
- packages/stack/src/bin/commands/init/steps/authenticate.ts
- packages/stack-forge/src/commands/index.ts
- packages/stack/src/bin/stash.ts
- packages/stack/src/bin/commands/auth/index.ts
- packages/stack-forge/README.md
- packages/stack/src/bin/commands/init/steps/build-schema.ts
- packages/stack-forge/src/bin/stash-forge.ts
| main().catch((err: unknown) => { | ||
| const message = err instanceof Error ? err.message : String(err) | ||
| p.log.error(`Fatal error: ${message}`) |
There was a problem hiding this comment.
Do not print raw fatal error messages.
Upstream exceptions here can carry connection strings, secret values, or other plaintext from user input. Emit a generic failure by default and keep detailed diagnostics behind an explicitly sanitized debug path.
As per coding guidelines, "Do NOT log plaintext; the library never logs plaintext by design and logs should never leak sensitive data."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/bin/stash.ts` around lines 247 - 249, The current
main().catch handler logs raw error content via p.log.error which may leak
secrets; change the handler in stash.ts so p.log.error emits a generic message
like "Fatal error: operation failed" (no raw error text) and send detailed
diagnostics only to an explicit debug channel—e.g., p.log.debug or a dedicated
sanitized logger—after sanitizing or redacting sensitive fields from the Error
object; update the catch block around main() and references to main() and
p.log.error/p.log.debug to implement generic user-facing logging plus a
controlled, sanitized debug path.
| env: { | ||
| ...process.env, | ||
| ANTHROPIC_BASE_URL: GATEWAY_URL, | ||
| ANTHROPIC_API_KEY: undefined, | ||
| }, |
There was a problem hiding this comment.
Avoid forwarding full process.env into the agent runtime.
...process.env can expose unrelated CI/local secrets to the spawned agent toolchain. Restrict to a minimal allowlist.
Suggested patch
+const AGENT_TEST_ENV = {
+ ANTHROPIC_BASE_URL: GATEWAY_URL,
+ // Keep only strictly required vars for subprocess/runtime behavior:
+ PATH: process.env.PATH,
+ HOME: process.env.HOME,
+ TMPDIR: process.env.TMPDIR,
+}
...
- env: {
- ...process.env,
- ANTHROPIC_BASE_URL: GATEWAY_URL,
- ANTHROPIC_API_KEY: undefined,
- },
+ env: AGENT_TEST_ENV,Also applies to: 129-133, 193-197, 257-261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/wizard/__tests__/agent-sdk.test.ts` around lines 65
- 69, The tests currently forward the entire process.env into spawned agent runs
(the env object that includes "...process.env"), which may leak CI/local
secrets; replace the spread with a minimal explicit allowlist: remove
"...process.env" and construct env objects containing only the variables
required for the agent runtime (e.g., ANTHROPIC_BASE_URL: GATEWAY_URL,
ANTHROPIC_API_KEY: undefined, plus any absolute minimum like PATH or NODE_ENV if
the child process needs them). Update every occurrence in this test file (the
env objects in agent-sdk.test.ts around the blocks that set
ANTHROPIC_BASE_URL/ANTHROPIC_API_KEY at lines shown) to use the allowlist
approach instead of spreading process.env.
| // The agent may or may not attempt curl — it's model-dependent | ||
| // But the response should acknowledge the limitation | ||
| expect(true).toBe(true) // test completes without hanging | ||
| } finally { |
There was a problem hiding this comment.
Test name promises enforcement, but no enforcement is asserted.
Line 299 (expect(true).toBe(true)) makes this pass even when canUseTool never denies anything. Please either assert denial (e.g., permissionDenied === true) or rename the test to reflect “does not hang” behavior.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/wizard/__tests__/agent-sdk.test.ts` around lines
297 - 300, The test currently uses a no-op assertion (expect(true).toBe(true))
which doesn't verify the intended enforcement; update the test in
agent-sdk.test.ts to assert that the tool permission was denied by replacing the
dummy assertion with an explicit check like expect(permissionDenied).toBe(true)
(or the actual boolean/flag produced by canUseTool in this test) so the test
enforces the denial behavior, or alternatively rename the test to indicate it
only ensures the flow "does not hang" if you prefer not to assert denial; locate
the assertion near the test block referencing canUseTool/permissionDenied and
change the expectation accordingly.
| it('returns "ready_with_warnings" when npm is degraded but gateway is up', async () => { | ||
| vi.mocked(fetch).mockImplementation(async (input) => { | ||
| const url = typeof input === 'string' ? input : input instanceof URL ? input.toString() : (input as Request).url | ||
| if (url.includes('npmjs')) { | ||
| return new Response(null, { status: 503 }) | ||
| } | ||
| return new Response(null, { status: 200 }) | ||
| }) | ||
| expect(await checkReadiness()).toBe('ready_with_warnings') | ||
| }) |
There was a problem hiding this comment.
This stub never drives the code down the warning path.
checkReadiness() only probes the gateway health URL in the shown implementation, so the url.includes('npmjs') branch is dead and this test currently exercises a 200 response instead. Align the mock with the actual checked endpoints, or add the missing non-blocking probe before asserting ready_with_warnings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/wizard/__tests__/health-checks.test.ts` around
lines 36 - 45, The test is stubbing fetch for the wrong URL so it never triggers
the warning branch in checkReadiness(); update the mock or test flow so the
mocked 503 is returned for the actual probe checkReadiness() performs (adjust
the fetch mock to inspect the gateway health URL that checkReadiness() calls and
return 200 for gateway but 503 for the npm/non-blocking probe), or explicitly
invoke the missing non-blocking npm probe before asserting 'ready_with_warnings'
so the code sees the degraded npm response; reference the mocked fetch and the
checkReadiness() call when making the change.
| it('blocks Stripe live keys in written content', () => { | ||
| const result = scanPostToolUseWrite('const key = "sk_live_abc123def456"') | ||
| expect(result.blocked).toBe(true) | ||
| expect(result.rule).toBe('hardcoded_stripe_key') | ||
| }) |
There was a problem hiding this comment.
Avoid literal sk_live_... fixtures to prevent secret-scanner failures.
This line matches real-token patterns and can fail leak detection in CI. Build the test string dynamically so behavior is preserved without embedding a scanner-triggering literal.
Suggested patch
- const result = scanPostToolUseWrite('const key = "sk_live_abc123def456"')
+ const stripeLike = `sk${'_live_'}abc123def456`
+ const result = scanPostToolUseWrite(`const key = "${stripeLike}"`)📝 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.
| it('blocks Stripe live keys in written content', () => { | |
| const result = scanPostToolUseWrite('const key = "sk_live_abc123def456"') | |
| expect(result.blocked).toBe(true) | |
| expect(result.rule).toBe('hardcoded_stripe_key') | |
| }) | |
| it('blocks Stripe live keys in written content', () => { | |
| const stripeLike = `sk${'_live_'}abc123def456` | |
| const result = scanPostToolUseWrite(`const key = "${stripeLike}"`) | |
| expect(result.blocked).toBe(true) | |
| expect(result.rule).toBe('hardcoded_stripe_key') | |
| }) |
🧰 Tools
🪛 Betterleaks (1.1.1)
[high] 68-68: Found a Stripe Access Token, posing a risk to payment processing services and sensitive financial data.
(stripe-access-token)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/wizard/__tests__/hooks.test.ts` around lines 67 -
71, The test uses a literal "sk_live_abc123def456" which triggers
secret-scanner; update the test that calls scanPostToolUseWrite (test named
'blocks Stripe live keys in written content') to construct the Stripe key string
dynamically (e.g., concatenate or interpolate 'sk_live_' with a non-sensitive
suffix generated in the test) so the produced value still matches the scanner
pattern but no literal sk_live_... appears in the source; keep the assertions
(result.blocked and result.rule === 'hardcoded_stripe_key') unchanged.
| if (message.type === 'result') { | ||
| turnCount++ | ||
|
|
||
| const isSuccess = (message.subtype === 'success' || message.subtype === 'error_max_turns') && !message.is_error |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Claude Agent SDK error_max_turns subtype behavior
💡 Result:
In the Claude Agent SDK, error_max_turns is a final result subtype that indicates the run stopped because it hit the configured max_turns limit (i.e., the allowed number of agentic tool-use “round trips”).[1][2]
What this means in practice:
- You’ll receive a
type: "result"(final) message whosesubtypeis"error_max_turns"(alongside other “budget/limit” result subtypes likeerror_max_budget_usd).[2] - In SDKs that expose an
is_error/error?helper,error_max_turnsis treated as an error outcome (even though it’s often a controlled stop condition rather than a crash).[2][4] - To avoid it, raise
max_turns(or design your agent to finish in fewer tool-use iterations);max_turnsis explicitly documented as “Maximum agentic turns (tool-use round trips)”.[1] - In streaming APIs, it appears as the end-of-session result subtype:
successvserror_max_turnsvserror_during_execution.[3]
Sources: Agent SDK Python reference (max_turns).[1] Agent SDK TypeScript reference (result subtype union including error_max_turns, plus is_error).[2] Elixir SDK “Result Subtypes” explanation.[3] Elixir SDK Message.error?/1 behavior.[4]
Remove error_max_turns from success condition on line 404.
The Claude Agent SDK treats error_max_turns as an error outcome, not a success case. The current logic ((message.subtype === 'success' || message.subtype === 'error_max_turns') && !message.is_error) incorrectly treats error_max_turns as success when is_error is false. Align with SDK behavior by only treating message.subtype === 'success' (without is_error flag) as success; handle error_max_turns as an error.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/wizard/agent/interface.ts` around lines 401 - 404,
The isSuccess computation incorrectly treats 'error_max_turns' as success;
update the logic in the block handling message.type === 'result' (where
turnCount is incremented and isSuccess is computed) to only consider
message.subtype === 'success' as success (i.e., remove the || message.subtype
=== 'error_max_turns' part) and ensure any handling for error_max_turns is
treated as an error path (use message.is_error or explicit subtype checks
elsewhere to handle error_max_turns).
| const controller = new AbortController() | ||
| const timeout = setTimeout( | ||
| () => controller.abort(), | ||
| HEALTH_CHECK_TIMEOUT_MS, | ||
| ) | ||
|
|
||
| const response = await fetch(url, { | ||
| method: 'GET', | ||
| signal: controller.signal, | ||
| }) | ||
|
|
||
| clearTimeout(timeout) | ||
|
|
There was a problem hiding this comment.
Always clear the abort timer on failures too.
If fetch() rejects before the explicit clearTimeout() call, the timer stays scheduled until it fires. That adds avoidable delay on fast failures and leaves stray timers behind across repeated readiness checks.
Suggested fix
async function checkEndpoint(
name: string,
url: string,
): Promise<HealthCheckResult> {
+ const controller = new AbortController()
+ const timeout = setTimeout(
+ () => controller.abort(),
+ HEALTH_CHECK_TIMEOUT_MS,
+ )
+
try {
- const controller = new AbortController()
- const timeout = setTimeout(
- () => controller.abort(),
- HEALTH_CHECK_TIMEOUT_MS,
- )
-
const response = await fetch(url, {
method: 'GET',
signal: controller.signal,
})
-
- clearTimeout(timeout)
if (response.ok) {
return { service: name, status: 'up' }
}
return {
service: name,
status: 'degraded',
message: `HTTP ${response.status}`,
}
} catch (error) {
return {
service: name,
status: 'down',
message: error instanceof Error ? error.message : 'Unknown error',
}
+ } finally {
+ clearTimeout(timeout)
}
}Also applies to: 34-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/wizard/health-checks/index.ts` around lines 12 -
24, The abort timer created with const controller = new AbortController() and
const timeout = setTimeout(... ) is only cleared after the await fetch(...)
succeeds, so on rejection the timeout remains scheduled; wrap the fetch call in
a try/finally (or ensure a finally block) so clearTimeout(timeout) is always
called and also move any controller.abort() logic accordingly; apply the same
change to the second occurrence around the other fetch block (the code using
controller.signal and clearTimeout(timeout) at lines shown) so timers are
cleared on both success and failure.
| const hasBlockingDown = checks.some( | ||
| (c) => BLOCKING_SERVICES.includes(c.service) && c.status === 'down', | ||
| ) | ||
|
|
||
| if (hasBlockingDown) return 'not_ready' | ||
|
|
||
| const hasAnyDegraded = checks.some((c) => c.status !== 'up') | ||
| if (hasAnyDegraded) return 'ready_with_warnings' |
There was a problem hiding this comment.
Treat degraded blocking services as not ready.
A blocking dependency returning HTTP 503 lands in 'degraded', and this predicate only blocks on 'down'. That lets the wizard continue even when the gateway health endpoint is explicitly failing.
Suggested fix
- const hasBlockingDown = checks.some(
- (c) => BLOCKING_SERVICES.includes(c.service) && c.status === 'down',
- )
-
- if (hasBlockingDown) return 'not_ready'
+ const hasBlockingUnavailable = checks.some(
+ (c) => BLOCKING_SERVICES.includes(c.service) && c.status !== 'up',
+ )
+
+ if (hasBlockingUnavailable) return 'not_ready'📝 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.
| const hasBlockingDown = checks.some( | |
| (c) => BLOCKING_SERVICES.includes(c.service) && c.status === 'down', | |
| ) | |
| if (hasBlockingDown) return 'not_ready' | |
| const hasAnyDegraded = checks.some((c) => c.status !== 'up') | |
| if (hasAnyDegraded) return 'ready_with_warnings' | |
| const hasBlockingUnavailable = checks.some( | |
| (c) => BLOCKING_SERVICES.includes(c.service) && c.status !== 'up', | |
| ) | |
| if (hasBlockingUnavailable) return 'not_ready' | |
| const hasAnyDegraded = checks.some((c) => c.status !== 'up') | |
| if (hasAnyDegraded) return 'ready_with_warnings' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/wizard/health-checks/index.ts` around lines 59 -
66, The code currently only treats blocking services with status 'down' as
blocking readiness; change the blocking check in hasBlockingDown to treat any
non-'up' status as blocking (e.g. (c) => BLOCKING_SERVICES.includes(c.service)
&& c.status !== 'up' or explicitly c.status === 'down' || c.status ===
'degraded') so that degraded blocking services also return 'not_ready'; keep the
subsequent hasAnyDegraded check as-is (it can still detect non-blocking degraded
services).
| missing.push( | ||
| 'Not authenticated with CipherStash. Run: npx stash auth login', | ||
| ) |
There was a problem hiding this comment.
Use a recovery command that resolves to this package.
npx stash auth login targets the stash package name, not @cipherstash/cli. That sends npx users down a broken recovery path right after the rename.
Suggested fix
- 'Not authenticated with CipherStash. Run: npx stash auth login',
+ 'Not authenticated with CipherStash. Run: npx `@cipherstash/cli` auth login',📝 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.
| missing.push( | |
| 'Not authenticated with CipherStash. Run: npx stash auth login', | |
| ) | |
| missing.push( | |
| 'Not authenticated with CipherStash. Run: npx `@cipherstash/cli` auth login', | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/wizard/lib/prerequisites.ts` around lines 21 - 23,
Update the recovery command string pushed into the missing array so it points to
the renamed package; replace the current literal "npx stash auth login" in the
missing.push(...) call with the command that resolves to this package (e.g. "npx
`@cipherstash/cli` auth login") so users are given a working auth login
instruction.
1 participant
Summary by CodeRabbit
New Features
Documentation
Chores