Merged
Conversation
🦋 Changeset detectedLatest commit: 3414761 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis PR introduces new CLI commands (init, validate, upgrade, status, test-connection) to stack-forge for interactive setup and configuration management, refactors SQL installation to use bundled EQL by default with a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 2
🧹 Nitpick comments (9)
packages/stack/README.md (1)
294-294: Clarify the client-side sort workaround for paginated queries.Sorting after decryption is only safe if the app has the full candidate set. With
LIMIT/pagination applied in SQL first, client-side sorting can return the wrong overall order. Please add that caveat so readers do not treat this as a drop-in replacement forORDER BY.Suggested wording
-On databases without operator families (e.g. Supabase, or when EQL is installed with `--exclude-operator-family`), sorting on encrypted columns is not currently supported — regardless of the client or ORM used. Sort application-side after decrypting the results as a workaround. +On databases without operator families (e.g. Supabase, or when EQL is installed with `--exclude-operator-family`), sorting on encrypted columns is not currently supported — regardless of the client or ORM used. As a workaround, decrypt and sort application-side only when you have the full candidate result set; if SQL `LIMIT`/pagination is applied first, the final order may be incorrect.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack/README.md` at line 294, Update the sentence about client-side sorting for encrypted columns to warn that sorting after decrypting is only correct when the client has the full candidate set; explicitly state that if the database query uses LIMIT, OFFSET or any server-side pagination (or applies filtering that reduces the candidate set before returning results), performing ORDER BY on the client after decryption can produce an incorrect global order, and recommend either removing server-side pagination before fetching all candidates or using a server-side sort supported by the DB/operator family as alternatives; modify the existing paragraph that begins "On databases without operator families..." to include this caveat and suggested remediation.packages/stack-forge/src/commands/upgrade.ts (2)
64-70: Same consideration for post-upgrade version check.The
getInstalledVersion()call at line 65 also lacks error handling. If verification fails, it might be better to warn the user rather than crash.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-forge/src/commands/upgrade.ts` around lines 64 - 70, The call to installer.getInstalledVersion() in upgrade flow should be made safe: wrap the getInstalledVersion() invocation (and the surrounding s.start/ s.stop instrumentation) in a try/catch so failures don't crash the process; on catch, call p.log.warn with a clear message referencing newVersion verification (include the caught error), set newVersion to undefined (or leave as null) so s.stop prints 'unknown', and continue to the existing comparison using previousVersion and newVersion (preserving the p.log.info('Version unchanged — EQL was already up to date.') path).
35-36: Missing error handling forgetInstalledVersion.If
getInstalledVersion()throws (e.g., connection issue between checks), the error propagates unhandled. Consider wrapping in try-catch for a graceful failure message, similar to the pattern used instatusCommand.♻️ Suggested improvement
- const previousVersion = await installer.getInstalledVersion() - s.stop(`Current version: ${previousVersion ?? 'unknown'}`) + let previousVersion: string | null = null + try { + previousVersion = await installer.getInstalledVersion() + s.stop(`Current version: ${previousVersion ?? 'unknown'}`) + } catch (error) { + s.stop('Failed to get current version.') + p.log.warn('Could not determine current version, continuing with upgrade...') + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-forge/src/commands/upgrade.ts` around lines 35 - 36, The call to installer.getInstalledVersion() in upgrade command lacks error handling so exceptions can bubble up; wrap the await installer.getInstalledVersion() in a try-catch, assign previousVersion inside the try, and on catch call s.stop with a helpful failure message (including the caught error) and return/exit similar to the pattern used in statusCommand; reference the installer.getInstalledVersion call and the previousVersion variable so you update that block only.packages/stack-forge/src/commands/push.ts (1)
13-28: Type assertion assumes valid input.The
as CastAsassertion on line 21 assumescolumn.cast_asis always a validCastAstype. IfEncryptConfigcomes from external/user input with an invalidcast_as, this could pass unexpected values totoEqlCastAs.If
EncryptConfigis already validated by the schema loader, this is fine. Otherwise, consider defensive validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-forge/src/commands/push.ts` around lines 13 - 28, The toEqlConfig function currently force-casts column.cast_as to CastAs before passing it to toEqlCastAs, which can propagate invalid user input; update toEqlConfig to defensively validate column.cast_as (from EncryptConfig) using a type guard or whitelist (e.g., check allowed CastAs values or switch) and either convert/normalize it or throw a clear error/replace with a safe default before calling toEqlCastAs; reference toEqlConfig, toEqlCastAs, EncryptConfig, CastAs and the column.cast_as usage so you can locate and replace the direct `as CastAs` assertion with the validation logic.packages/stack-forge/src/commands/status.ts (1)
99-107: Fragile error detection via substring matching.Checking
message.includes('does not exist')is brittle — different PostgreSQL versions or locales could phrase this differently. Consider catching the specific PostgreSQL error code instead.♻️ Suggested improvement using PostgreSQL error codes
} catch (error) { s.stop('Configuration check failed.') - // The table may not exist if push has never been run — that's fine - const message = error instanceof Error ? error.message : String(error) - if (message.includes('does not exist')) { + // The table may not exist if push has never been run — that's fine + // PostgreSQL error code 42P01 = undefined_table + const pgError = error as { code?: string; message?: string } + if (pgError.code === '42P01') { p.log.info( 'Active encrypt config: table not found (run `stash-forge push` to create it)', ) } else { + const message = error instanceof Error ? error.message : String(error) p.log.error(`Failed to check encrypt configuration: ${message}`) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-forge/src/commands/status.ts` around lines 99 - 107, Replace the fragile substring check of the error message with a check of the Postgres error code (undefined_table = '42P01') instead: when handling the caught error in the status command, cast the error to any or to the Postgres error type and test error.code === '42P01' (or check typeof error.code === 'string' before comparing) to detect "table not found" and call p.log.info(...) in that case; otherwise fall back to p.log.error(...) for other errors. Use the existing variables (error, message, p.log) and update the conditional around the message/includes('does not exist') to the code-based check.packages/stack-forge/src/installer/index.ts (1)
314-341: Code duplication betweendownloadInstallScriptanddownloadEqlSql.The private
downloadInstallScriptmethod and the exporteddownloadEqlSqlfunction have nearly identical implementations. Consider having the private method delegate to the exported function to reduce duplication.♻️ Suggested refactor
private async downloadInstallScript( excludeOperatorFamily: boolean, ): Promise<string> { - const url = excludeOperatorFamily - ? EQL_INSTALL_NO_OPERATOR_FAMILY_URL - : EQL_INSTALL_URL - - let response: Response - - try { - response = await fetch(url) - } catch (error) { - throw new Error('Failed to download EQL install script from GitHub.', { - cause: error, - }) - } - - if (!response.ok) { - throw new Error( - `Failed to download EQL install script from GitHub. HTTP ${response.status}: ${response.statusText}`, - ) - } - - return response.text() + return downloadEqlSql(excludeOperatorFamily) }Also applies to: 385-413
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-forge/src/installer/index.ts` around lines 314 - 341, Refactor to remove duplication by having the class private method downloadInstallScript(excludeOperatorFamily) delegate to the existing exported function downloadEqlSql(excludeOperatorFamily): replace the duplicate fetch/error handling inside downloadInstallScript with a call to downloadEqlSql and return its result, preserving the excludeOperatorFamily boolean and letting downloadEqlSql throw the same errors; ensure any differences in error wrapping or Response handling are reconciled by updating downloadEqlSql (if needed) so callers (including downloadInstallScript) receive consistent behavior.skills/stash-forge/SKILL.md (1)
157-164: Minor formatting: Add blank line before table.The markdown linter flags that tables should be surrounded by blank lines for better compatibility with various Markdown parsers.
📝 Proposed fix
**Flags:** + | Flag | Description | |------|-------------|Apply similar fixes at lines 183 and 189.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/stash-forge/SKILL.md` around lines 157 - 164, Add a blank line before the Markdown table that follows the "Flags:" heading so the table is separated from the preceding paragraph; similarly add blank lines before the other two tables referenced in the file (the tables near the sections corresponding to the occurrences noted at lines ~183 and ~189) to satisfy the markdown linter and ensure consistent parsing.packages/stack-forge/src/bin/stash-forge.ts (1)
80-83: Hardcoded version string.The version
'0.1.0'is hardcoded. Consider reading frompackage.jsonto keep the version in sync automatically.💡 Suggested improvement
import { readFileSync } from 'node:fs' import { fileURLToPath } from 'node:url' import { dirname, join } from 'node:path' // Near the top of the file or in a helper function getVersion(): string { const __dirname = dirname(fileURLToPath(import.meta.url)) const pkg = JSON.parse(readFileSync(join(__dirname, '../../package.json'), 'utf-8')) return pkg.version }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-forge/src/bin/stash-forge.ts` around lines 80 - 83, Replace the hardcoded version string in the CLI exit branch (the if checking flags.version || command === '--version' || command === '-v') with a dynamic read from package.json; add a helper like getVersion (or similar) that computes __dirname via fileURLToPath(import.meta.url) and dirname, reads and parses the package.json (using node:fs readFileSync or fs/promises), returns pkg.version, and call that helper instead of printing '0.1.0' so the CLI output stays in sync with package.json.packages/stack-forge/src/commands/init.ts (1)
461-464: Top-levelawaitin generated code.The generated encryption client uses top-level
await(export const encryptionClient = await Encryption({...})). This requires the consuming project to have"type": "module"inpackage.jsonor use.mjsextension. Consider documenting this requirement or generating an async factory function instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/stack-forge/src/commands/init.ts` around lines 461 - 464, The generated code uses top-level await for "export const encryptionClient = await Encryption({...})", which forces consumers to use ESM; change this to an async factory to avoid breaking projects: replace the top-level await with an exported async function (e.g., "export async function createEncryptionClient() { return Encryption({ schemas: [${schemaVarName}] }); }" or "export const createEncryptionClient = async () => Encryption({...});"), update any internal references to use createEncryptionClient(), and update generated docs/comments to mention the async factory symbol instead of a direct constant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/stack-forge/tsup.config.ts`:
- Around line 21-24: The post-build hook onSuccess currently uses
cpSync('src/sql', 'dist/sql', { recursive: true }) but doesn't remove stale
files; import and use rmSync to remove or empty the destination first—e.g., call
rmSync('dist/sql', { recursive: true, force: true }) (or rmSync followed by
mkdir if needed) before cpSync—so update the top-level imports to include rmSync
and modify the onSuccess block (the onSuccess function using cpSync) to clear
'dist/sql' prior to copying from 'src/sql'.
In `@packages/stack/src/supabase/helpers.ts`:
- Around line 208-225: formatOrValue and parseOrValue mishandle internal double
quotes: when a string matches POSTGREST_RESERVED and is wrapped in double
quotes, internal " characters must be escaped as \" and conversely parseOrValue
must unescape \" back to " when stripping outer quotes. Update formatOrValue to
replace all internal double quotes with escaped quotes before surrounding with
outer quotes (preserving existing POSTGREST_RESERVED check), and update
parseOrValue to detect a value wrapped in double quotes, remove the outer
quotes, and replace escaped sequences (\") with actual double quotes.
---
Nitpick comments:
In `@packages/stack-forge/src/bin/stash-forge.ts`:
- Around line 80-83: Replace the hardcoded version string in the CLI exit branch
(the if checking flags.version || command === '--version' || command === '-v')
with a dynamic read from package.json; add a helper like getVersion (or similar)
that computes __dirname via fileURLToPath(import.meta.url) and dirname, reads
and parses the package.json (using node:fs readFileSync or fs/promises), returns
pkg.version, and call that helper instead of printing '0.1.0' so the CLI output
stays in sync with package.json.
In `@packages/stack-forge/src/commands/init.ts`:
- Around line 461-464: The generated code uses top-level await for "export const
encryptionClient = await Encryption({...})", which forces consumers to use ESM;
change this to an async factory to avoid breaking projects: replace the
top-level await with an exported async function (e.g., "export async function
createEncryptionClient() { return Encryption({ schemas: [${schemaVarName}] });
}" or "export const createEncryptionClient = async () => Encryption({...});"),
update any internal references to use createEncryptionClient(), and update
generated docs/comments to mention the async factory symbol instead of a direct
constant.
In `@packages/stack-forge/src/commands/push.ts`:
- Around line 13-28: The toEqlConfig function currently force-casts
column.cast_as to CastAs before passing it to toEqlCastAs, which can propagate
invalid user input; update toEqlConfig to defensively validate column.cast_as
(from EncryptConfig) using a type guard or whitelist (e.g., check allowed CastAs
values or switch) and either convert/normalize it or throw a clear error/replace
with a safe default before calling toEqlCastAs; reference toEqlConfig,
toEqlCastAs, EncryptConfig, CastAs and the column.cast_as usage so you can
locate and replace the direct `as CastAs` assertion with the validation logic.
In `@packages/stack-forge/src/commands/status.ts`:
- Around line 99-107: Replace the fragile substring check of the error message
with a check of the Postgres error code (undefined_table = '42P01') instead:
when handling the caught error in the status command, cast the error to any or
to the Postgres error type and test error.code === '42P01' (or check typeof
error.code === 'string' before comparing) to detect "table not found" and call
p.log.info(...) in that case; otherwise fall back to p.log.error(...) for other
errors. Use the existing variables (error, message, p.log) and update the
conditional around the message/includes('does not exist') to the code-based
check.
In `@packages/stack-forge/src/commands/upgrade.ts`:
- Around line 64-70: The call to installer.getInstalledVersion() in upgrade flow
should be made safe: wrap the getInstalledVersion() invocation (and the
surrounding s.start/ s.stop instrumentation) in a try/catch so failures don't
crash the process; on catch, call p.log.warn with a clear message referencing
newVersion verification (include the caught error), set newVersion to undefined
(or leave as null) so s.stop prints 'unknown', and continue to the existing
comparison using previousVersion and newVersion (preserving the
p.log.info('Version unchanged — EQL was already up to date.') path).
- Around line 35-36: The call to installer.getInstalledVersion() in upgrade
command lacks error handling so exceptions can bubble up; wrap the await
installer.getInstalledVersion() in a try-catch, assign previousVersion inside
the try, and on catch call s.stop with a helpful failure message (including the
caught error) and return/exit similar to the pattern used in statusCommand;
reference the installer.getInstalledVersion call and the previousVersion
variable so you update that block only.
In `@packages/stack-forge/src/installer/index.ts`:
- Around line 314-341: Refactor to remove duplication by having the class
private method downloadInstallScript(excludeOperatorFamily) delegate to the
existing exported function downloadEqlSql(excludeOperatorFamily): replace the
duplicate fetch/error handling inside downloadInstallScript with a call to
downloadEqlSql and return its result, preserving the excludeOperatorFamily
boolean and letting downloadEqlSql throw the same errors; ensure any differences
in error wrapping or Response handling are reconciled by updating downloadEqlSql
(if needed) so callers (including downloadInstallScript) receive consistent
behavior.
In `@packages/stack/README.md`:
- Line 294: Update the sentence about client-side sorting for encrypted columns
to warn that sorting after decrypting is only correct when the client has the
full candidate set; explicitly state that if the database query uses LIMIT,
OFFSET or any server-side pagination (or applies filtering that reduces the
candidate set before returning results), performing ORDER BY on the client after
decryption can produce an incorrect global order, and recommend either removing
server-side pagination before fetching all candidates or using a server-side
sort supported by the DB/operator family as alternatives; modify the existing
paragraph that begins "On databases without operator families..." to include
this caveat and suggested remediation.
In `@skills/stash-forge/SKILL.md`:
- Around line 157-164: Add a blank line before the Markdown table that follows
the "Flags:" heading so the table is separated from the preceding paragraph;
similarly add blank lines before the other two tables referenced in the file
(the tables near the sections corresponding to the occurrences noted at lines
~183 and ~189) to satisfy the markdown linter and ensure consistent parsing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ddfd23c9-ffda-4940-881b-6471a16bcd0d
📒 Files selected for processing (31)
.changeset/sharp-phones-own.md.changeset/wet-zoos-follow.mdexamples/basic/src/encryption/index.tsexamples/basic/stash.config.tspackages/stack-forge/README.mdpackages/stack-forge/package.jsonpackages/stack-forge/src/__tests__/installer.test.tspackages/stack-forge/src/bin/stash-forge.tspackages/stack-forge/src/commands/index.tspackages/stack-forge/src/commands/init.tspackages/stack-forge/src/commands/install.tspackages/stack-forge/src/commands/push.tspackages/stack-forge/src/commands/status.tspackages/stack-forge/src/commands/test-connection.tspackages/stack-forge/src/commands/upgrade.tspackages/stack-forge/src/commands/validate.tspackages/stack-forge/src/config/index.tspackages/stack-forge/src/index.tspackages/stack-forge/src/installer/index.tspackages/stack-forge/src/sql/cipherstash-encrypt-no-operator-family.sqlpackages/stack-forge/src/sql/cipherstash-encrypt-supabase.sqlpackages/stack-forge/src/sql/cipherstash-encrypt.sqlpackages/stack-forge/tsup.config.tspackages/stack/README.mdpackages/stack/__tests__/schema-builders.test.tspackages/stack/src/schema/index.tspackages/stack/src/supabase/helpers.tsskills/stash-drizzle/SKILL.mdskills/stash-encryption/SKILL.mdskills/stash-forge/SKILL.mdskills/stash-supabase/SKILL.md
💤 Files with no reviewable changes (1)
- packages/stack-forge/src/config/index.ts
Comment on lines
+21
to
+24
| onSuccess: async () => { | ||
| // Copy bundled SQL files into dist so they ship with the package | ||
| cpSync('src/sql', 'dist/sql', { recursive: true }) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd packages/stack-forge && head -40 tsup.config.tsRepository: cipherstash/stack
Length of output: 1147
🌐 Web query:
Node.js fs.cpSync behavior overwrite stale files
💡 Result:
fs.cpSync() does not try to be “smart” about staleness (mtime/size/contents). It simply copies, and its overwrite behavior is controlled by options:
- Default is overwrite:
force: trueby default, meaning existing destination paths are overwritten. If you setforce: falseand the destination exists, Node will ignore the conflict unless you also seterrorOnExist: trueto throw instead. [1] - It will not delete “stale” destination files (files that exist in
destbut no longer exist insrc). It only copies what’s insrc. (So “stale files” remain unless you remove them yourself.) [1]
To ensure dest exactly matches src, do a wipe-then-copy:
import fs from "node:fs";
fs.rmSync(dest, { recursive: true, force: true });
fs.cpSync(src, dest, { recursive: true, force: true });fs.rmSync(..., { recursive: true, force: true }) is the Node-provided equivalent of rm -rf. [1]
Sources:
[1] Node.js File System docs (fs.cpSync, force, errorOnExist, fs.rmSync) https://nodejs.org/api/fs.html
[2] Exploring JS summary of fs.cpSync options (force default true, errorOnExist) https://2ality.com/2022/06/nodejs-file-system.html
Clear dist/sql before copying into it.
fs.cpSync() only overwrites files it copies from the source; it leaves destination-only files untouched. Deleted or renamed files under src/sql will persist in dist/sql across rebuilds, potentially causing stale SQL assets to ship with the package. Update the import on line 1 to include rmSync and wipe the destination directory before copying.
♻️ Suggested change
-import { cpSync } from 'node:fs'
+import { cpSync, rmSync } from 'node:fs' onSuccess: async () => {
// Copy bundled SQL files into dist so they ship with the package
+ rmSync('dist/sql', { recursive: true, force: true })
cpSync('src/sql', 'dist/sql', { recursive: true })
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/stack-forge/tsup.config.ts` around lines 21 - 24, The post-build
hook onSuccess currently uses cpSync('src/sql', 'dist/sql', { recursive: true })
but doesn't remove stale files; import and use rmSync to remove or empty the
destination first—e.g., call rmSync('dist/sql', { recursive: true, force: true
}) (or rmSync followed by mkdir if needed) before cpSync—so update the top-level
imports to include rmSync and modify the onSuccess block (the onSuccess function
using cpSync) to clear 'dist/sql' prior to copying from 'src/sql'.
Comment on lines
208
to
+225
| function formatOrValue(value: unknown): string { | ||
| if (Array.isArray(value)) { | ||
| return `(${value.join(',')})` | ||
| } | ||
| if (value === null) return 'null' | ||
| if (value === true) return 'true' | ||
| if (value === false) return 'false' | ||
| return String(value) | ||
|
|
||
| const str = String(value) | ||
|
|
||
| // Wrap in double quotes if the value contains reserved characters. | ||
| // This is required for encrypted values (JSON with commas, braces, etc.) | ||
| // and is safe for all string values per PostgREST spec. | ||
| if (POSTGREST_RESERVED.test(str)) { | ||
| return `"${str}"` | ||
| } | ||
|
|
||
| return str |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
How does PostgREST handle escaped double quotes within quoted filter values?
💡 Result:
PostgREST lets you escape a double quote inside a double-quoted filter value with a backslash (\"). If you need a literal backslash, escape it as \\. This is mainly relevant when you’re using double-quoted values to protect reserved characters (often in in.(...)). [1][2]
Example (URL-encoded form shown in the docs):
- A value containing
":Quote:"is sent as%22Quote:%5C%22%22(because\is%5Cand"is%22). [1] - A value containing
\:Backslash:\is sent as%22Backslash:%5C%5C%22. [1]
Sources:
[1] PostgREST docs (Reserved characters / escaping in filters) (docs.postgrest.org)
[2] PostgREST release notes v9.0.0 (added support for escaping quotes in double-quoted strings) (postgrest.org)
Citations:
🏁 Script executed:
# Search for formatOrValue usage and POSTGREST_RESERVED definition
rg -n "formatOrValue|POSTGREST_RESERVED" packages/stack/src/supabase/helpers.ts -B 2 -A 2Repository: cipherstash/stack
Length of output: 697
🏁 Script executed:
# Check for any existing escaping or sanitization of double quotes
rg -n "escape|sanitize|quote" packages/stack/src/supabase/helpers.tsRepository: cipherstash/stack
Length of output: 215
🏁 Script executed:
# Look for test cases or examples showing expected behavior
fd "*.test\|*.spec" packages/stack/src/supabase/ --exec grep -l "formatOrValue\|POSTGREST" {} \;Repository: cipherstash/stack
Length of output: 354
🏁 Script executed:
# Find test files with correct syntax
fd -e test.ts -e spec.ts -e test.js packages/stack/src/supabase/Repository: cipherstash/stack
Length of output: 478
🏁 Script executed:
# Look at the broader context where formatOrValue is used
rg -B 10 "formatOrValue(c.value)" packages/stack/src/supabase/helpers.ts | head -30Repository: cipherstash/stack
Length of output: 43
🏁 Script executed:
# Check parseOrValue to understand the inverse operation
sed -n '178,201p' packages/stack/src/supabase/helpers.tsRepository: cipherstash/stack
Length of output: 634
🏁 Script executed:
# Check if there's any escaping logic elsewhere in the file
rg -n "\\\\\"" packages/stack/src/supabase/helpers.tsRepository: cipherstash/stack
Length of output: 43
🏁 Script executed:
# Look at the complete formatOrValue function
sed -n '208,226p' packages/stack/src/supabase/helpers.tsRepository: cipherstash/stack
Length of output: 602
🏁 Script executed:
# Search for any test files related to these helper functions
fd . 'packages/stack/src/supabase/' -e test.ts -e spec.tsRepository: cipherstash/stack
Length of output: 43
🏁 Script executed:
# Check if there are usage examples or test patterns
rg "formatOrValue|parseOrValue" packages/stack/src/ --type ts -A 3 -B 1 | head -50Repository: cipherstash/stack
Length of output: 1445
Fix double quote escaping in formatOrValue and parseOrValue to prevent PostgREST parsing errors.
The current implementation does not escape internal double quotes within values that contain reserved characters. PostgREST requires backslash escaping: a double quote inside a double-quoted value must be escaped as \".
For example, a value like foo,"bar" (containing both a comma and internal quotes) currently produces "foo,"bar" which will fail PostgREST parsing. It should produce "foo,\"bar\"".
Update formatOrValue to escape internal double quotes before wrapping:
if (POSTGREST_RESERVED.test(str)) {
return `"${str.replace(/"/g, '\\"')}"`
}Also update parseOrValue to unescape internal quotes when stripping the outer quotes:
if (value.startsWith('"') && value.endsWith('"')) {
return value.slice(1, -1).replace(/\\"/g, '"')
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/stack/src/supabase/helpers.ts` around lines 208 - 225, formatOrValue
and parseOrValue mishandle internal double quotes: when a string matches
POSTGREST_RESERVED and is wrapped in double quotes, internal " characters must
be escaped as \" and conversely parseOrValue must unescape \" back to " when
stripping outer quotes. Update formatOrValue to replace all internal double
quotes with escaped quotes before surrounding with outer quotes (preserving
existing POSTGREST_RESERVED check), and update parseOrValue to detect a value
wrapped in double quotes, remove the outer quotes, and replace escaped sequences
(\") with actual double quotes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
--latestflag to download latest EQL SQL from GitHub instead of using bundled versionBug Fixes
Documentation