feat: add experimental plugin API and AWS auth plugin#452
Merged
Conversation
### What changed - Experimental plugin system for extending SafeQL with custom behavior - First official plugin: `@ts-safeql/plugin-auth-aws` for AWS RDS IAM authentication - Docs and demo project for plugin usage ### Why Enables modular extensibility—users can add plugins for custom authentication, connection logic, and future hooks without hardcoding every scenario. Keeps SafeQL’s worker-based design while making it extensible.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
🦋 Changeset detectedLatest commit: f4d3e9b The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 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 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an experimental plugin system: new plugin-utils (definePlugin, PluginManager, test driver), connection-manager for plugin-sourced connections/caching, ESLint plugin integration for plugin hooks and worker errors, an official AWS IAM plugin and demo, zod-annotator package, multiple tests, docs, and package manifests/ build configs. Changes
Sequence Diagram(s)sequenceDiagram
participant ESLint as ESLint Rule
participant PM as PluginManager
participant CM as ConnectionManager
participant Plugin as Plugin Module
participant DB as Database
ESLint->>PM: resolvePlugins(descriptors, projectDir)
PM->>Plugin: load factory(descriptor.config) -> SafeQLPlugin
Plugin-->>PM: SafeQLPlugin instance
PM-->>ESLint: resolved plugins
ESLint->>PM: resolvePluginTargetMatch(plugins, node, context)
alt matched
PM-->>ESLint: TargetMatch
else skip/undef
PM-->>ESLint: false/undefined
end
ESLint->>CM: getOrCreateFromPlugins(plugins, projectDir)
CM->>PM: resolveConnection(descriptors)
PM->>Plugin: createConnection.handler()
Plugin->>DB: obtain credentials/token (e.g., AWS IAM)
DB-->>Plugin: token
Plugin-->>CM: Postgres client
CM-->>ESLint: ConnectionPayload { sql, pluginName, isFirst }
ESLint->>Plugin: onExpression({ node, context })
alt fragment returned
Plugin-->>ESLint: SQL fragment
else skip/undef
Plugin-->>ESLint: false/undefined
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~80 minutes Possibly related PRs
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)
📝 Coding Plan
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 |
Greptile SummaryThis PR introduces an experimental plugin API for SafeQL that allows users to extend connection handling via a descriptor/factory pattern — necessary because the worker thread boundary requires serializable data — and ships the first official plugin, Key findings:
Confidence Score: 3/5
Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (8)
docs/plugins/auth-aws.md (1)
11-13: Prefer installing this plugin as a dev dependency in docs.Since this package is used by ESLint config/tooling,
-Dis usually the better default in the installation snippet.📝 Suggested docs tweak
-pnpm add `@ts-safeql/plugin-auth-aws` +pnpm add -D `@ts-safeql/plugin-auth-aws`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/plugins/auth-aws.md` around lines 11 - 13, Update the installation snippet to recommend adding the plugin as a dev dependency: change the current pnpm add command for `@ts-safeql/plugin-auth-aws` to use the dev flag (pnpm add -D or pnpm add --save-dev) so the docs show installing the package as a development dependency used by ESLint/tooling..agents/docs/creating-package.md (1)
7-8: Add a package-level verification step after install.Include a final step to
cdinto the new package and run tests there; this avoids common command-context mistakes.Based on learnings: Always navigate to the package directory before running tests with pnpm and vitest.✅ Suggested addition
5. Run `pnpm install` 6. Workspace glob `packages/plugins/*` is already in `pnpm-workspace.yaml` +7. Validate from the package directory: + - `cd packages/<pkg>` (or `cd packages/plugins/<slug>`) + - `pnpm test -- --run`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/docs/creating-package.md around lines 7 - 8, Add a final verification step after "Run `pnpm install`" that instructs the user to change directory into the newly created package and run the package-level tests (e.g., cd into the package folder and run `pnpm test` or the project's vitest command) to ensure tests run in the correct context; reference the existing steps around "Run `pnpm install`" and the workspace globs in `pnpm-workspace.yaml` so the reader knows exactly where to add this step.docs/guide/plugins.md (2)
17-38: Add a security note about plugin trust and execution.Since plugins execute code during lint/worker runtime, a short warning to install only trusted packages (and pin versions) would strengthen the guidance.
🔐 Suggested note
## Using Plugins +::: warning SECURITY +Plugins execute JavaScript in your lint/worker process. Install only trusted plugins and keep versions pinned in your lockfile. +::: + Install the plugin package alongside `@ts-safeql/eslint-plugin`, then add it to the `plugins` array in your connection config:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide/plugins.md` around lines 17 - 38, Add a brief security note warning users that plugins run code during lint/worker runtime and advising to install only trusted packages, pin plugin versions, and audit plugin sources; place this note near the example and mention the plugins array, createConnection hook, databaseUrl, and migrationsDir so readers understand that plugins loaded in the plugins array (including any createConnection hook) execute at runtime and should be trusted and version-pinned.
99-100: Tighten naming convention wording for scoped packages.The current convention text can be read as preferring
safeql-plugin-*, while this PR’s official package uses@ts-safeql/plugin-*. Consider explicitly listing both accepted patterns.🧭 Suggested wording
-- Plugin packages should be named `safeql-plugin-<name>` or scoped under your org. +- Plugin packages should use either `safeql-plugin-<name>` (unscoped) or `@<scope>/plugin-<name>` (scoped, e.g. `@ts-safeql/plugin-auth-aws`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/guide/plugins.md` around lines 99 - 100, Update the plugin naming guidance to list both accepted patterns explicitly: state that plugin packages may be named either safeql-plugin-<name> or use the scoped org form `@ts-safeql/plugin-`<name>, and also note that either form should include safeql-plugin in the package.json "keywords" field; edit the text replacing the ambiguous sentence that currently prefers safeql-plugin-* with a clear line enumerating both accepted patterns (mentioning safeql-plugin-<name> and `@ts-safeql/plugin-`<name>) and keep the instruction to include "safeql-plugin" in package.json keywords..agents/docs/authoring-plugin.md (1)
5-16: Clarify<name>vs package naming to avoid ambiguity.The checklist and constraints are good, but
<name>can be read as either full package suffix or raw plugin slug. A short explicit mapping would make authoring less error-prone.✏️ Suggested clarification
-- [ ] Create a new package under `packages/plugins/<name>/` (see [creating a package](creating-package.md)) +- [ ] Create a new package under `packages/plugins/<slug>/` (see [creating a package](creating-package.md)) ... -- Package names must be prefixed with `plugin-` (e.g., `@ts-safeql/plugin-auth-aws`). +- Package names must be `@ts-safeql/plugin-<slug>` (e.g., `@ts-safeql/plugin-auth-aws`).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/docs/authoring-plugin.md around lines 5 - 16, Clarify that the placeholder "packages/plugins/<name>/" refers to the raw plugin slug (no prefixes) and explicitly show mapping examples: slug -> package folder (packages/plugins/<slug>/), npm package name must be prefixed with "plugin-" (e.g., plugin-<slug>) and the published plugin name is auto-prefixed with "safeql-plugin-" (resulting in `@ts-safeql/plugin-`<slug>); update the checklist lines containing "packages/plugins/<name>/", "plugin-" and "safeql-plugin-" to include this explicit mapping and an example so authors know to use type aliases for config and to default-export a definePlugin() result.demos/plugin-aws-iam/src/index.ts (1)
3-10: CI failure is expected behavior for this demo.The pipeline failure "Could not load credentials from any providers" is intentional — this demo validates that the plugin surfaces credential errors with the correct
[safeql-plugin-aws-iam]prefix rather than generic internal errors. The accompanyingverify-plugin-error.shscript validates this behavior.However, the function lacks proper cleanup on error. If the query fails after the connection is established,
sql.end()won't be called.♻️ Suggested improvement for cleanup
export async function queryUsers() { const sql = postgres(); - - const users = await sql<{ id: number }[]>`SELECT 1 as id`; - - await sql.end(); - return users; + try { + const users = await sql<{ id: number }[]>`SELECT 1 as id`; + return users; + } finally { + await sql.end(); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/plugin-aws-iam/src/index.ts` around lines 3 - 10, queryUsers currently may skip closing the DB connection if the query throws; wrap the usage of postgres() / sql and the query in a try/finally so that await sql.end() is always called (only if sql is truthy), ensuring cleanup on both success and error; update the function (referencing queryUsers, postgres, sql.end) to acquire sql, run the query inside try, and perform await sql.end() in finally with a null check.packages/plugins/auth-aws/package.json (1)
32-37: Consider tightening AWS SDK version ranges.The
^3.0.0ranges for AWS SDK packages are extremely loose, allowing any version from 3.0.0 to <4.0.0. The latest versions of both@aws-sdk/credential-providersand@aws-sdk/rds-signerare at 3.1011.0, indicating substantial evolution since the 3.0.0 release. While AWS SDK v3 maintains backward compatibility, this wide range could lead to inconsistent behavior across different environments or build timestamps.Consider pinning to a more specific minor version range (e.g.,
^3.1011.0) to ensure consistent builds while still allowing patch updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/auth-aws/package.json` around lines 32 - 37, The dependency ranges in packages/plugins/auth-aws/package.json for "@aws-sdk/credential-providers" and "@aws-sdk/rds-signer" are too loose (currently "^3.0.0"); update those entries to a tighter minor-pinned range (for example "^3.1011.0" or the current tested minor version) so builds remain consistent across environments, keeping the caret to allow patch updates but preventing unexpected minor-version jumps; modify the dependencies block in package.json accordingly and run your lockfile/install to verify.demos/plugin-aws-iam/eslint.config.js (1)
19-26: Consider adding validation for required environment variables.Empty string fallbacks for required configuration fields (host, user, database, region) will cause unclear errors at runtime. For a demo, adding explicit checks or comments indicating which env vars are required would improve developer experience.
💡 Optional: Add validation helper
// Add at the top of the file: function requireEnv(name) { const value = process.env[name]; if (!value) throw new Error(`Missing required env var: ${name}`); return value; } // Then use: awsIamAuth({ databaseHost: requireEnv("SAFEQL_DATABASE_HOST"), // ... })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@demos/plugin-aws-iam/eslint.config.js` around lines 19 - 26, The awsIamAuth config currently uses empty-string fallbacks for critical env vars which hides missing-config errors; add a small validation helper (e.g., requireEnv) and replace the fallbacks for SAFEQL_DATABASE_HOST, SAFEQL_DATABASE_USER, SAFEQL_DATABASE_NAME and SAFEQL_AWS_REGION with requireEnv("...") calls so missing values throw a clear error at startup; for SAFEQL_DATABASE_PORT either requireEnv then Number(...) or keep a documented default but validate numeric conversion; leave SAFEQL_AWS_PROFILE as optional if intended.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@demos/plugin-aws-iam/.env.example`:
- Around line 1-6: Reorder the environment keys in the .env.example so they
follow dotenv-linter's expected alphabetical order to eliminate UnorderedKey
warnings; specifically arrange the keys SAFEQL_AWS_PROFILE, SAFEQL_AWS_REGION,
SAFEQL_DATABASE_HOST, SAFEQL_DATABASE_NAME, SAFEQL_DATABASE_PORT,
SAFEQL_DATABASE_USER (or follow the project's canonical ordering rule) so the
file lists these exact symbols in the correct sorted sequence.
In `@demos/plugin-aws-iam/scripts/verify-plugin-error.sh`:
- Around line 13-15: The checks use `grep -qv` incorrectly (it succeeds if any
line doesn't match), so update the negative checks in verify-plugin-error.sh to
use a negated grep status instead: replace the `echo "$output" | grep -qv
'Internal error'` and `echo "$output" | grep -qv 'could not be loaded'` checks
with `! grep -q` style invocations (or equivalent `! echo "$output" | grep -q`)
so they only succeed when the patterns "Internal error" and "could not be
loaded" are completely absent from $output; keep the positive check for
'\[safeql-plugin-aws-iam\]' as-is.
In `@docs/api/index.md`:
- Line 95: Replace the invalid in-page link fragments `#connections-databaseurl`
and `#connections-migrationsdir` on the sentence that mentions
`createConnection` with the correct anchor IDs (or remove the links) so
markdownlint MD051 is satisfied; locate the occurrence of those fragments in the
docs line referencing `createConnection` and update them to the actual heading
anchors used in the document (or convert to plain text) to ensure the links are
not dead.
In `@packages/eslint-plugin/src/rules/RuleOptions.ts`:
- Around line 61-64: The rule's Zod schema uses z.object({}).loose(), which is
not available in Zod v4; update the zPluginDescriptor schema to replace the
invalid call by using z.looseObject({}) for the config field (i.e., change the
config entry from z.object({}).loose() to z.looseObject({})) so
zPluginDescriptor and its use of z.object remain compatible with Zod v4.
In `@packages/eslint-plugin/src/utils/connection-manager.ts`:
- Around line 46-72: Wrap the entire plugin resolution and connection creation
logic inside getOrCreateFromPlugins (both the call to
pluginManager.resolveConnection and the handler() invocation) in a single
try-catch so any error thrown during plugin loading/validation is converted to a
PluginError via PluginError.from(pluginName); obtain pluginName from the
resolved connection when available (fall back to a generic identifier if
resolveConnection throws before a connection object exists) and rethrow
PluginError.from(pluginName)(error), keeping the existing connectionMap/cacheKey
handling intact.
In `@packages/eslint-plugin/src/utils/plugin-test-driver.ts`:
- Around line 57-63: connectionHook currently injects
source.createConnection.cacheKey as raw code but cacheKey is a string; update
the generation in plugin-test-driver.ts so that connectionHook emits cacheKey as
a string literal (e.g., wrap/serialize source.createConnection.cacheKey) instead
of raw code; locate the template that builds createConnection (the
connectionHook variable using source.createConnection.cacheKey and
source.createConnection.handler) and ensure cacheKey is properly
quoted/JSON-stringified while preserving the existing handler injection.
- Line 6: The import ConnectionPayload in
packages/eslint-plugin/src/utils/plugin-test-driver.ts is unused and causing CI
failure; remove the unused import line (the "ConnectionPayload" named import) or
alternatively use it in an appropriate type annotation where connection payloads
are handled (e.g., functions in plugin-test-driver that accept or construct
connection objects). Prefer simply deleting the "ConnectionPayload" import if no
type usage is intended.
- Around line 85-89: In the connect method ensure you guard against an empty
descriptors array before accessing descriptors[0].package: check
descriptors.length and if zero either require/validate that explicitProjectDir
is provided or throw a clear error; then compute projectDir only after that
check and proceed to call createConnectionManager() and
manager.getOrCreateFromPlugins(descriptors, projectDir). This involves updating
connect (symbols: connect, descriptors, projectDir, descriptors[0].package,
createConnectionManager, getOrCreateFromPlugins) to validate inputs and fail
fast or use the provided projectDir when descriptors is empty.
In `@packages/plugin-utils/src/resolve.ts`:
- Around line 99-105: The plugin load currently only validates plugin.name;
extend validation in the withFactory usage (where SafeQLPlugin is asserted) to
also check the shape of createConnection: verify that plugin.createConnection
exists and is either a function or an object with a callable handler property
(e.g., typeof plugin.createConnection === "function" or (typeof
plugin.createConnection === "object" && typeof plugin.createConnection.handler
=== "function")), and throw a clear Error if not; this ensures code in
connection-manager.ts that does plugin.createConnection().handler() or
plugin.createConnection.handler() won't crash later.
- Around line 61-63: getCacheKey currently serializes the descriptor.config with
only top-level sorted keys which drops nested properties and can cause cache
collisions; update the getCacheKey method to use a deep stable serialization of
PluginDescriptor.config (e.g., a deterministic canonical/JSON serializer that
recursively sorts object keys and handles arrays/primitives consistently) and
include descriptor.package as before so that different nested configs produce
unique keys for the cache.
In `@packages/plugins/auth-aws/src/index.ts`:
- Line 23: The cacheKey generation in packages/plugins/auth-aws/src/index.ts
only uses database connection fields (cacheKey:
`iam://${config.databaseUser}@${config.databaseHost}:${port}/${config.databaseName}`)
which can collide across different AWS setups; update the cacheKey to also
include AWS-specific config values (e.g., config.awsRegion and config.awsProfile
or the AWS role/ARN field your plugin uses) so keys are unique per AWS
configuration while preserving existing DB fields (databaseUser, databaseHost,
port, databaseName).
---
Nitpick comments:
In @.agents/docs/authoring-plugin.md:
- Around line 5-16: Clarify that the placeholder "packages/plugins/<name>/"
refers to the raw plugin slug (no prefixes) and explicitly show mapping
examples: slug -> package folder (packages/plugins/<slug>/), npm package name
must be prefixed with "plugin-" (e.g., plugin-<slug>) and the published plugin
name is auto-prefixed with "safeql-plugin-" (resulting in
`@ts-safeql/plugin-`<slug>); update the checklist lines containing
"packages/plugins/<name>/", "plugin-" and "safeql-plugin-" to include this
explicit mapping and an example so authors know to use type aliases for config
and to default-export a definePlugin() result.
In @.agents/docs/creating-package.md:
- Around line 7-8: Add a final verification step after "Run `pnpm install`" that
instructs the user to change directory into the newly created package and run
the package-level tests (e.g., cd into the package folder and run `pnpm test` or
the project's vitest command) to ensure tests run in the correct context;
reference the existing steps around "Run `pnpm install`" and the workspace globs
in `pnpm-workspace.yaml` so the reader knows exactly where to add this step.
In `@demos/plugin-aws-iam/eslint.config.js`:
- Around line 19-26: The awsIamAuth config currently uses empty-string fallbacks
for critical env vars which hides missing-config errors; add a small validation
helper (e.g., requireEnv) and replace the fallbacks for SAFEQL_DATABASE_HOST,
SAFEQL_DATABASE_USER, SAFEQL_DATABASE_NAME and SAFEQL_AWS_REGION with
requireEnv("...") calls so missing values throw a clear error at startup; for
SAFEQL_DATABASE_PORT either requireEnv then Number(...) or keep a documented
default but validate numeric conversion; leave SAFEQL_AWS_PROFILE as optional if
intended.
In `@demos/plugin-aws-iam/src/index.ts`:
- Around line 3-10: queryUsers currently may skip closing the DB connection if
the query throws; wrap the usage of postgres() / sql and the query in a
try/finally so that await sql.end() is always called (only if sql is truthy),
ensuring cleanup on both success and error; update the function (referencing
queryUsers, postgres, sql.end) to acquire sql, run the query inside try, and
perform await sql.end() in finally with a null check.
In `@docs/guide/plugins.md`:
- Around line 17-38: Add a brief security note warning users that plugins run
code during lint/worker runtime and advising to install only trusted packages,
pin plugin versions, and audit plugin sources; place this note near the example
and mention the plugins array, createConnection hook, databaseUrl, and
migrationsDir so readers understand that plugins loaded in the plugins array
(including any createConnection hook) execute at runtime and should be trusted
and version-pinned.
- Around line 99-100: Update the plugin naming guidance to list both accepted
patterns explicitly: state that plugin packages may be named either
safeql-plugin-<name> or use the scoped org form `@ts-safeql/plugin-`<name>, and
also note that either form should include safeql-plugin in the package.json
"keywords" field; edit the text replacing the ambiguous sentence that currently
prefers safeql-plugin-* with a clear line enumerating both accepted patterns
(mentioning safeql-plugin-<name> and `@ts-safeql/plugin-`<name>) and keep the
instruction to include "safeql-plugin" in package.json keywords.
In `@docs/plugins/auth-aws.md`:
- Around line 11-13: Update the installation snippet to recommend adding the
plugin as a dev dependency: change the current pnpm add command for
`@ts-safeql/plugin-auth-aws` to use the dev flag (pnpm add -D or pnpm add
--save-dev) so the docs show installing the package as a development dependency
used by ESLint/tooling.
In `@packages/plugins/auth-aws/package.json`:
- Around line 32-37: The dependency ranges in
packages/plugins/auth-aws/package.json for "@aws-sdk/credential-providers" and
"@aws-sdk/rds-signer" are too loose (currently "^3.0.0"); update those entries
to a tighter minor-pinned range (for example "^3.1011.0" or the current tested
minor version) so builds remain consistent across environments, keeping the
caret to allow patch updates but preventing unexpected minor-version jumps;
modify the dependencies block in package.json accordingly and run your
lockfile/install to verify.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b84cb2f9-5a5e-49fa-ab41-9dab8fcd4640
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (49)
.agents/docs/authoring-plugin.md.agents/docs/creating-package.md.changeset/plugin-system.md.gitignoreAGENTS.mddemos/basic-flat-config/package.jsondemos/basic-migrations-raw/package.jsondemos/basic-transform-type/package.jsondemos/basic/package.jsondemos/big-project/package.jsondemos/config-file-flat-config/package.jsondemos/from-config-file/package.jsondemos/multi-connections/package.jsondemos/playground/package.jsondemos/plugin-aws-iam/.env.exampledemos/plugin-aws-iam/eslint.config.jsdemos/plugin-aws-iam/package.jsondemos/plugin-aws-iam/scripts/verify-plugin-error.shdemos/plugin-aws-iam/src/index.tsdemos/plugin-aws-iam/tsconfig.jsondemos/postgresjs-custom-types/package.jsondemos/postgresjs-demo/package.jsondemos/shared/package.jsondocs/.vitepress/config.tsdocs/api/index.mddocs/guide/plugins.mddocs/plugins/auth-aws.mdpackages/eslint-plugin/package.jsonpackages/eslint-plugin/src/rules/RuleOptions.tspackages/eslint-plugin/src/rules/check-sql.rule.tspackages/eslint-plugin/src/rules/check-sql.utils.tspackages/eslint-plugin/src/utils/connection-manager.test.tspackages/eslint-plugin/src/utils/connection-manager.tspackages/eslint-plugin/src/utils/plugin-test-driver.tspackages/eslint-plugin/src/workers/check-sql.worker.tspackages/generate/package.jsonpackages/plugin-utils/build.config.tspackages/plugin-utils/package.jsonpackages/plugin-utils/src/index.tspackages/plugin-utils/src/resolve.tspackages/plugin-utils/tsconfig.jsonpackages/plugins/auth-aws/build.config.tspackages/plugins/auth-aws/package.jsonpackages/plugins/auth-aws/src/index.tspackages/plugins/auth-aws/tsconfig.jsonpackages/shared/package.jsonpackages/shared/src/errors.tspackages/test-utils/package.jsonpnpm-workspace.yaml
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
packages/plugin-utils/src/resolve.ts (2)
69-72:⚠️ Potential issue | 🔴 Critical
getCacheKeycan collide for different nested configs.Using
JSON.stringify(config, Object.keys(config).sort())only sorts top-level keys and drops nested properties from the replacer array. Distinct plugin configurations with different nested values can produce the same cache key, causing the wrong cached plugin instance to be reused.🔧 Use deep stable serialization for cache keys
private getCacheKey(descriptor: PluginDescriptor): string { - const config = descriptor.config ?? {}; - return `${descriptor.package}:${JSON.stringify(config, Object.keys(config).sort())}`; + const stable = (value: unknown): unknown => { + if (Array.isArray(value)) return value.map(stable); + if (value && typeof value === "object") { + return Object.fromEntries( + Object.entries(value as Record<string, unknown>) + .sort(([a], [b]) => a.localeCompare(b)) + .map(([k, v]) => [k, stable(v)]), + ); + } + return value; + }; + + return `${descriptor.package}:${JSON.stringify(stable(descriptor.config ?? {}))}`; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-utils/src/resolve.ts` around lines 69 - 72, The getCacheKey method currently only sorts top-level keys via JSON.stringify with Object.keys(config).sort(), which can produce identical keys for different nested configs; change getCacheKey (in the function named getCacheKey and related to PluginDescriptor) to use a deep stable serializer instead of the top-level replacer—either import/use a library like fast-json-stable-stringify or implement a helper that recursively sorts object keys (deeply normalize the config) and then stringify that normalized object so nested properties are included deterministically in the cache key.
139-145:⚠️ Potential issue | 🟠 MajorValidate
createConnectionshape during plugin load.The current validation only checks
plugin.name. If a plugin returns a malformedcreateConnection(e.g., missingcacheKeyorhandler), the failure occurs later whenconnection-managerinvokeshandler(), making debugging harder.🔧 Add runtime contract checks for plugin hooks
const plugin = withFactory(descriptor.config ?? {}) as SafeQLPlugin; if (!plugin || typeof plugin.name !== "string") { throw new Error( `SafeQL plugin "${descriptor.package}" factory must return an object with at least a "name" property.`, ); } + if ( + plugin.createConnection && + (typeof plugin.createConnection.cacheKey !== "string" || + typeof plugin.createConnection.handler !== "function") + ) { + throw new Error( + `SafeQL plugin "${descriptor.package}" returned invalid "createConnection". Expected { cacheKey: string; handler: () => Promise<Sql> }.`, + ); + } + return plugin;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-utils/src/resolve.ts` around lines 139 - 145, The plugin validation currently only checks plugin.name; extend it to also validate the createConnection hook shape when loading the plugin returned by withFactory(descriptor.config ?? {}) (typed as SafeQLPlugin) by ensuring plugin.createConnection exists and is an object/array (as expected) and that each connection entry provides the required fields (e.g., cacheKey string and handler function). Update the validation near the existing check for plugin.name to throw a clear error referencing descriptor.package and the missing/malformed createConnection contract (so connection-manager will not fail later when invoking handler). Ensure you reference the createConnection property on the plugin returned by withFactory and validate handler is a function and cacheKey is present and a string.packages/connection-manager/src/test-driver.ts (2)
52-58:⚠️ Potential issue | 🟡 Minor
cacheKeyis emitted as code, not as a string literal.
PluginSource.createConnection.cacheKeyis typed asstring, but line 55 injects it raw into the generated code. This works when callers pass JavaScript expressions (e.g.,'"test://" + config.databaseUrl'), but the type contract suggests a plain string value should work. Consider documenting this expectation or usingJSON.stringifyfor literal cacheKeys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/connection-manager/src/test-driver.ts` around lines 52 - 58, The generated connectionHook injects source.createConnection.cacheKey as raw code which breaks the typed contract (cacheKey: string); update the template to emit a proper string literal by using JSON.stringify(source.createConnection.cacheKey) when building connectionHook (reference: connectionHook, source.createConnection.cacheKey, createConnection.handler) so callers can pass plain string values; alternatively document the current expectation if you prefer expressions.
80-84:⚠️ Potential issue | 🟡 MinorGuard empty descriptors before accessing
descriptors[0].If
descriptorsis empty andprojectDiris not provided,descriptors[0].packagethrows a runtime error. Consider adding a guard or requiringprojectDirwhen descriptors are empty.🔧 Suggested guard
async connect(params: { descriptors: PluginDescriptor[]; projectDir?: string }) { const { descriptors, projectDir: explicitProjectDir } = params; + if (!explicitProjectDir && descriptors.length === 0) { + throw new Error("connect() requires at least one plugin descriptor when projectDir is omitted."); + } const manager = createConnectionManager(); const projectDir = explicitProjectDir ?? path.dirname(descriptors[0].package);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/connection-manager/src/test-driver.ts` around lines 80 - 84, The connect method accesses descriptors[0].package without ensuring descriptors is non-empty; when descriptors is empty and explicitProjectDir is undefined this will throw. Update the connect function to guard for an empty descriptors array: if explicitProjectDir is provided use it, otherwise require or throw a clear error when descriptors.length === 0, or compute projectDir from descriptors[0] only after confirming descriptors.length > 0; keep references to connect, descriptors, projectDir, createConnectionManager, and getOrCreateFromPlugins so the logic feeds the validated projectDir into manager.getOrCreateFromPlugins.
🧹 Nitpick comments (7)
packages/connection-manager/src/pg.utils.ts (1)
18-20: Inconsistent null check forhostvs other fields.
connection.hostis checked with=== null, while other fields useisDefined()which checks for bothnullandundefined. According topg-connection-stringtypes,hostcan also beundefined. Consider usingisDefinedconsistently for all fields.♻️ Proposed fix for consistency
- if (connection.host === null) { + if (!isDefined(connection.host)) { throw new Error("Could not resolve database host"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/connection-manager/src/pg.utils.ts` around lines 18 - 20, The null check for connection.host is inconsistent with other fields; replace the strict === null check with the existing isDefined() utility to cover both null and undefined (e.g., change the host check to if (!isDefined(connection.host)) { throw new Error("Could not resolve database host"); }) so host validation matches the other fields such as those already using isDefined().packages/plugin-utils/src/resolve.ts (1)
82-82: Inconsistent path construction between sync and async resolution.
resolveOneSyncusespath.resolve(projectDir, "package.json")whileresolveOneusespath.join(projectDir, "package.json"). Both should behave the same for consistency and predictability.♻️ Proposed fix for consistency
- const projectRequire = createRequire(path.resolve(projectDir, "package.json")); + const projectRequire = createRequire(path.join(projectDir, "package.json"));Also applies to: 109-109
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-utils/src/resolve.ts` at line 82, resolveOneSync and resolveOne construct the package.json path differently causing inconsistent behavior; standardize both to use the same path API. Update the code where projectRequire is created (the createRequire call in resolveOne and the similar call in resolveOneSync) to use path.resolve(projectDir, "package.json") in both places (also fix the second occurrence around the other createRequire at the later instance), ensuring resolveOne and resolveOneSync behave identically when locating package.json.packages/eslint-plugin/src/rules/connection-strategy.test.ts (1)
5-24: Consider adding test coverage for additional strategy scenarios.The test only covers the case where both
databaseUrlandpluginsare present. According togetConnectionStartegyByRuleOptionConnection, the function also handles:
pluginsOnlystrategy (plugins without databaseUrl/migrationsDir)migrationsstrategy with plugins- Error case (no databaseUrl/migrationsDir/plugins)
♻️ Suggested additional test cases
it("plugins-only — returns pluginsOnly strategy", () => { const connection = { plugins: [{ package: "some-plugin", config: {} }], targets: [{ tag: "sql" as const }], }; const strategy = getConnectionStartegyByRuleOptionConnection({ connection, projectDir: os.tmpdir(), }); expect(strategy.type).toBe("pluginsOnly"); expect(strategy.plugins).toEqual([{ package: "some-plugin", config: {} }]); }); it("throws when no connection method is specified", () => { const connection = { targets: [{ tag: "sql" as const }], }; expect(() => getConnectionStartegyByRuleOptionConnection({ connection, projectDir: os.tmpdir(), }), ).toThrow("must specify databaseUrl, migrationsDir, or plugins"); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eslint-plugin/src/rules/connection-strategy.test.ts` around lines 5 - 24, Add tests covering the missing connection strategy branches for getConnectionStartegyByRuleOptionConnection: add a "plugins-only" case that supplies only plugins and asserts strategy.type === "pluginsOnly" and strategy.plugins matches, add a "migrations" case if applicable that provides migrationsDir (and plugins) and asserts strategy.type === "migrations" and plugins preserved, and add an error case that supplies neither databaseUrl nor migrationsDir nor plugins and asserts the function throws the expected message; locate and extend connection-strategy.test.ts near the existing "plugins coexist with databaseUrl" test and reference getConnectionStartegyByRuleOptionConnection to exercise each branch.packages/plugins/auth-aws/src/index.ts (1)
26-32: Redundant destructuring ofawsProfile.
awsProfileis destructured fromconfigat line 31, but the code uses theprofilevariable (from line 20) instead. This shadowed variable is unused and creates confusion. The fix for the credential provider issue above would also make this destructuring meaningful again.♻️ Clean up by using config.awsProfile directly for the check
const { databaseHost: hostname, databaseUser: username, awsRegion: region, - awsProfile: profile, } = config; - const credentials = profile + const credentials = config.awsProfile ? fromIni({ profile }) : fromNodeProviderChain({ clientConfig: { region } });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/auth-aws/src/index.ts` around lines 26 - 32, The handler function currently destructures awsProfile as profile from config but then uses an outer profile variable, causing a shadowed and unused variable; update the handler in the async handler() block so you either remove awsProfile from the destructuring or reference config.awsProfile directly (or rename to avoid collision) and ensure any credential-provider logic uses that single source (awsProfile/profile) consistently; adjust references in the credential provider code paths so awsProfile is not shadowed and is actually used..agents/docs/authoring-plugin.md (1)
49-58: Add language identifier to fenced code block.The directory structure code block is missing a language identifier. Use
textorplaintextfor non-code content.📝 Proposed fix
-``` +```text packages/plugins/my-lib/ ├── src/ │ ├── index.ts # definePlugin() export🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.agents/docs/authoring-plugin.md around lines 49 - 58, The fenced directory-structure block in authoring-plugin.md is missing a language identifier; update the triple-backtick that opens the block (the block showing "packages/plugins/my-lib/ ... build.config.ts") to include a plaintext language tag (e.g., ```text or ```plaintext) so the diagram renders as plain text in docs.packages/zod-annotator/src/test-driver.ts (1)
110-137: Consider using ESLint's built-in traversal utilities.The manual AST traversal works correctly but could be simplified using
TSESTreetraversal utilities oresqueryif available. The current implementation is functional, so this is optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/zod-annotator/src/test-driver.ts` around lines 110 - 137, The manual recursive traversal in findFirstTaggedTemplate is working but can be simplified and made more robust by replacing it with ESLint/TypeScript-ESTree traversal utilities (e.g., context.getSourceCode().ast and eslint-scope or `@typescript-eslint/experimental-utils`' AST walker) or using esquery to query for 'TaggedTemplateExpression' nodes; update findFirstTaggedTemplate to leverage those utilities to locate and return the first TaggedTemplateExpression (or undefined) instead of manually iterating object keys and arrays, keeping the same return semantics and ensuring you reference the same function name findFirstTaggedTemplate in the change.packages/plugin-utils/package.json (1)
39-41: Consider using caret range for postgres dependency.The
postgresdependency is pinned to an exact version (3.4.7). If intentional for strict compatibility, this is fine. Otherwise, consider using a caret range (^3.4.7) to allow compatible updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-utils/package.json` around lines 39 - 41, Update the postgres dependency entry in package.json: replace the exact version string "3.4.7" for the "postgres" dependency with a caret range "^3.4.7" to allow compatible semver minor/patch updates (or explicitly leave it pinned if strict locking was intended); target the "dependencies" object and the "postgres" key when making this change.
🤖 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/connection-manager/src/index.ts`:
- Around line 110-150: The closeConnection function currently calls sql.end()
without awaiting its Promise; make closeConnection async (returning
Promise<void>) and await each sql.end() invocation before calling
connectionMap.delete() so deletions occur after closure; update all branches
(databaseUrl, migrations where both sql and migrationSql are awaited, and
pluginsOnly with pluginManager.getCachedConnection/cacheKey handling) to await
end(), and also update createConnectionManager's type/signature to reflect the
async closeConnection (and any callers) so callers can await cleanup completion.
In `@packages/connection-manager/src/pg.utils.ts`:
- Around line 11-13: mapConnectionOptionsToString currently interpolates
connectionOptions.password (and user) directly into the PostgreSQL URL which
breaks when those values contain reserved URI characters; update
mapConnectionOptionsToString to percent-encode the username and password (e.g.,
via encodeURIComponent) before building the `postgres://...` string, and ensure
you handle null/undefined by defaulting to empty strings so the generated URL is
always valid.
In `@packages/plugin-utils/src/plugin-test-driver.ts`:
- Around line 73-97: The test driver currently silently returns { skipped: true
} when typed ParserServices are unavailable, which hides broken project/tsconfig
setups; update the logic in plugin-test-driver.ts (around the uses of
ParserServices, checker, nodeMap, parserServices, and the
onTarget/taggedTemplate selection) to fail fast: detect when services.program or
services.esTreeNodeToTSNodeMap is missing and throw a descriptive Error
(mentioning missing typed parser services and projectDir/tsconfig) before
proceeding to findAllTaggedTemplates or invoking this.plugin.onTarget; apply the
same guard and error throw to the similar block referenced at the later location
(the code around lines 110-115).
- Around line 87-93: The loop over allTemplates calls this.plugin.onTarget with
targetCtx and overwrites taggedTemplate for every match, causing later matches
to silently override earlier ones; change the logic in plugin-test-driver so
that when plugin.onTarget returns a non-undefined/non-false result you set
taggedTemplate and then either break out of the loop to keep the first match
(preferred for deterministic behavior) or, if you want to enforce uniqueness,
throw an error if taggedTemplate is already set (detect a second match) — update
the loop that iterates allTemplates and references plugin.onTarget,
taggedTemplate, and targetCtx to implement one of these behaviors.
In `@packages/plugins/auth-aws/src/index.ts`:
- Around line 34-36: The credential selection always picks fromIni because local
variable profile is defaulted to "default"; change the logic to detect whether
the user explicitly passed awsProfile instead of using the defaulted profile
variable: use the original config field (awsProfile) presence to decide between
fromIni({ profile }) and fromNodeProviderChain({ clientConfig: { region } }), or
set profile to undefined when awsProfile is not provided so the ternary (profile
? fromIni(...) : fromNodeProviderChain(...)) behaves correctly; update the code
that constructs credentials (the credentials variable) and references to
profile, keeping fromIni and fromNodeProviderChain calls intact.
In `@packages/zod-annotator/src/annotator.ts`:
- Around line 43-49: The autofix must replace only the schema argument instead
of reprinting the entire call: keep the original callee text (calleeText), any
type arguments, all other call arguments and their comments/formatting, and only
swap the existing schema expression with the new zodStr produced by
resolvedTargetToZodSchema; build the replacement text by reading the existing
typeParameters and arguments from node.tag (via sourceCode.getText on those
subnodes), construct a new argument list that substitutes the schema argument
with zodStr while preserving other arguments and spacing, and return that as the
fix text instead of always emitting `${calleeText}(${zodStr})`.
- Around line 77-123: Add explicit checks in tsTypeToTarget for TypeScript
literal flags (ts.TypeFlags.StringLiteral, NumberLiteral, BooleanLiteral,
BigIntLiteral) before the final fallback to checker.typeToString so literal
types produce a "literal" target instead of a "type" target; extract the literal
value from the type (cast to ts.LiteralType and read .value) and return { kind:
"literal", value: literalValue } for those cases (update the branches in
tsTypeToTarget that currently handle primitives/unions/objects and the final
fallback).
In `@packages/zod-annotator/src/test-driver.ts`:
- Around line 85-92: The test driver directly instantiates TSESLint.SourceCode
(in the call to annotator with sourceCode: new TSESLint.SourceCode(source,
ast)), which is not officially supported in `@typescript-eslint` v8; replace this
with a supported approach by either (a) using the official ESLint SourceCode
class from the 'eslint' package (import and instantiate it correctly) or (b)
supplying a lightweight stub/mock implementing the minimal SourceCode surface
your annotator() needs (text, getText, ast node utilities) and pass that stub as
sourceCode; update the test-driver so annotator(...) receives the supported
SourceCode instance or mock instead of new TSESLint.SourceCode(...).
---
Duplicate comments:
In `@packages/connection-manager/src/test-driver.ts`:
- Around line 52-58: The generated connectionHook injects
source.createConnection.cacheKey as raw code which breaks the typed contract
(cacheKey: string); update the template to emit a proper string literal by using
JSON.stringify(source.createConnection.cacheKey) when building connectionHook
(reference: connectionHook, source.createConnection.cacheKey,
createConnection.handler) so callers can pass plain string values; alternatively
document the current expectation if you prefer expressions.
- Around line 80-84: The connect method accesses descriptors[0].package without
ensuring descriptors is non-empty; when descriptors is empty and
explicitProjectDir is undefined this will throw. Update the connect function to
guard for an empty descriptors array: if explicitProjectDir is provided use it,
otherwise require or throw a clear error when descriptors.length === 0, or
compute projectDir from descriptors[0] only after confirming descriptors.length
> 0; keep references to connect, descriptors, projectDir,
createConnectionManager, and getOrCreateFromPlugins so the logic feeds the
validated projectDir into manager.getOrCreateFromPlugins.
In `@packages/plugin-utils/src/resolve.ts`:
- Around line 69-72: The getCacheKey method currently only sorts top-level keys
via JSON.stringify with Object.keys(config).sort(), which can produce identical
keys for different nested configs; change getCacheKey (in the function named
getCacheKey and related to PluginDescriptor) to use a deep stable serializer
instead of the top-level replacer—either import/use a library like
fast-json-stable-stringify or implement a helper that recursively sorts object
keys (deeply normalize the config) and then stringify that normalized object so
nested properties are included deterministically in the cache key.
- Around line 139-145: The plugin validation currently only checks plugin.name;
extend it to also validate the createConnection hook shape when loading the
plugin returned by withFactory(descriptor.config ?? {}) (typed as SafeQLPlugin)
by ensuring plugin.createConnection exists and is an object/array (as expected)
and that each connection entry provides the required fields (e.g., cacheKey
string and handler function). Update the validation near the existing check for
plugin.name to throw a clear error referencing descriptor.package and the
missing/malformed createConnection contract (so connection-manager will not fail
later when invoking handler). Ensure you reference the createConnection property
on the plugin returned by withFactory and validate handler is a function and
cacheKey is present and a string.
---
Nitpick comments:
In @.agents/docs/authoring-plugin.md:
- Around line 49-58: The fenced directory-structure block in authoring-plugin.md
is missing a language identifier; update the triple-backtick that opens the
block (the block showing "packages/plugins/my-lib/ ... build.config.ts") to
include a plaintext language tag (e.g., ```text or ```plaintext) so the diagram
renders as plain text in docs.
In `@packages/connection-manager/src/pg.utils.ts`:
- Around line 18-20: The null check for connection.host is inconsistent with
other fields; replace the strict === null check with the existing isDefined()
utility to cover both null and undefined (e.g., change the host check to if
(!isDefined(connection.host)) { throw new Error("Could not resolve database
host"); }) so host validation matches the other fields such as those already
using isDefined().
In `@packages/eslint-plugin/src/rules/connection-strategy.test.ts`:
- Around line 5-24: Add tests covering the missing connection strategy branches
for getConnectionStartegyByRuleOptionConnection: add a "plugins-only" case that
supplies only plugins and asserts strategy.type === "pluginsOnly" and
strategy.plugins matches, add a "migrations" case if applicable that provides
migrationsDir (and plugins) and asserts strategy.type === "migrations" and
plugins preserved, and add an error case that supplies neither databaseUrl nor
migrationsDir nor plugins and asserts the function throws the expected message;
locate and extend connection-strategy.test.ts near the existing "plugins coexist
with databaseUrl" test and reference getConnectionStartegyByRuleOptionConnection
to exercise each branch.
In `@packages/plugin-utils/package.json`:
- Around line 39-41: Update the postgres dependency entry in package.json:
replace the exact version string "3.4.7" for the "postgres" dependency with a
caret range "^3.4.7" to allow compatible semver minor/patch updates (or
explicitly leave it pinned if strict locking was intended); target the
"dependencies" object and the "postgres" key when making this change.
In `@packages/plugin-utils/src/resolve.ts`:
- Line 82: resolveOneSync and resolveOne construct the package.json path
differently causing inconsistent behavior; standardize both to use the same path
API. Update the code where projectRequire is created (the createRequire call in
resolveOne and the similar call in resolveOneSync) to use
path.resolve(projectDir, "package.json") in both places (also fix the second
occurrence around the other createRequire at the later instance), ensuring
resolveOne and resolveOneSync behave identically when locating package.json.
In `@packages/plugins/auth-aws/src/index.ts`:
- Around line 26-32: The handler function currently destructures awsProfile as
profile from config but then uses an outer profile variable, causing a shadowed
and unused variable; update the handler in the async handler() block so you
either remove awsProfile from the destructuring or reference config.awsProfile
directly (or rename to avoid collision) and ensure any credential-provider logic
uses that single source (awsProfile/profile) consistently; adjust references in
the credential provider code paths so awsProfile is not shadowed and is actually
used.
In `@packages/zod-annotator/src/test-driver.ts`:
- Around line 110-137: The manual recursive traversal in findFirstTaggedTemplate
is working but can be simplified and made more robust by replacing it with
ESLint/TypeScript-ESTree traversal utilities (e.g., context.getSourceCode().ast
and eslint-scope or `@typescript-eslint/experimental-utils`' AST walker) or using
esquery to query for 'TaggedTemplateExpression' nodes; update
findFirstTaggedTemplate to leverage those utilities to locate and return the
first TaggedTemplateExpression (or undefined) instead of manually iterating
object keys and arrays, keeping the same return semantics and ensuring you
reference the same function name findFirstTaggedTemplate in the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 329b977c-2833-4173-967a-205b53e58b49
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (33)
.agents/docs/authoring-plugin.md.agents/docs/coding-guidelines.mddemos/plugin-aws-iam/.env.exampledemos/plugin-aws-iam/scripts/verify-plugin-error.shdocs/guide/plugins.mdpackages/connection-manager/build.config.tspackages/connection-manager/package.jsonpackages/connection-manager/src/index.test.tspackages/connection-manager/src/index.tspackages/connection-manager/src/pg.utils.tspackages/connection-manager/src/test-driver.tspackages/connection-manager/tsconfig.jsonpackages/eslint-plugin/package.jsonpackages/eslint-plugin/src/config.tspackages/eslint-plugin/src/rules/RuleOptions.tspackages/eslint-plugin/src/rules/check-sql.rule.tspackages/eslint-plugin/src/rules/check-sql.utils.tspackages/eslint-plugin/src/rules/connection-strategy.test.tspackages/eslint-plugin/src/utils/watch-manager.tspackages/eslint-plugin/src/workers/check-sql.worker.tspackages/plugin-utils/build.config.tspackages/plugin-utils/package.jsonpackages/plugin-utils/src/index.tspackages/plugin-utils/src/plugin-test-driver.tspackages/plugin-utils/src/resolve.tspackages/plugins/auth-aws/src/index.tspackages/zod-annotator/build.config.tspackages/zod-annotator/package.jsonpackages/zod-annotator/src/annotator.test.tspackages/zod-annotator/src/annotator.tspackages/zod-annotator/src/index.tspackages/zod-annotator/src/test-driver.tspackages/zod-annotator/tsconfig.json
✅ Files skipped from review due to trivial changes (8)
- packages/zod-annotator/tsconfig.json
- .agents/docs/coding-guidelines.md
- packages/connection-manager/tsconfig.json
- packages/eslint-plugin/package.json
- packages/zod-annotator/src/index.ts
- packages/zod-annotator/package.json
- packages/connection-manager/package.json
- docs/guide/plugins.md
🚧 Files skipped from review as they are similar to previous changes (6)
- demos/plugin-aws-iam/.env.example
- demos/plugin-aws-iam/scripts/verify-plugin-error.sh
- packages/plugin-utils/build.config.ts
- packages/eslint-plugin/src/rules/check-sql.utils.ts
- packages/plugin-utils/src/index.ts
- packages/eslint-plugin/src/rules/check-sql.rule.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/plugins/auth-aws/src/plugin.test.ts (1)
11-28: Consider adding test coverage for default values.The test verifies all explicit config fields appear in the cacheKey, but doesn't test the default behavior when
databasePortandawsProfileare omitted. This would ensure the defaults (5432and"default") are applied correctly.🧪 Suggested additional test case
it("cache key uses defaults when optional fields omitted", () => { const instance = plugin.factory(BASE_CONFIG); const key = instance.createConnection?.cacheKey; expect(key).toContain(":5432/"); // default port expect(key).toContain("profile=default"); // default profile });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugins/auth-aws/src/plugin.test.ts` around lines 11 - 28, Add a new unit test that calls plugin.factory with just BASE_CONFIG (omitting databasePort and awsProfile) and asserts the produced instance.createConnection?.cacheKey contains the default port and profile values; specifically ensure the key includes ":5432" (or ":5432/" if your cacheKey formats a slash) and "profile=default" (or the exact substring your cacheKey uses for awsProfile), referencing plugin.factory, BASE_CONFIG, and instance.createConnection?.cacheKey to locate and implement the test.
🤖 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/plugin-utils/src/resolve.ts`:
- Line 81: Both sync and async resolvers use inconsistent path handling: update
resolveOne (the async path) to mirror resolveOneSync by constructing
projectRequire with createRequire(path.resolve(projectDir, "package.json"))
instead of path.join so projectDir is normalized to an absolute path; modify the
code that creates projectRequire in the resolveOne function (and any other place
still using path.join with projectDir) to use path.resolve(projectDir,
"package.json") to ensure consistent module resolution behavior between
resolveOneSync and resolveOne.
---
Nitpick comments:
In `@packages/plugins/auth-aws/src/plugin.test.ts`:
- Around line 11-28: Add a new unit test that calls plugin.factory with just
BASE_CONFIG (omitting databasePort and awsProfile) and asserts the produced
instance.createConnection?.cacheKey contains the default port and profile
values; specifically ensure the key includes ":5432" (or ":5432/" if your
cacheKey formats a slash) and "profile=default" (or the exact substring your
cacheKey uses for awsProfile), referencing plugin.factory, BASE_CONFIG, and
instance.createConnection?.cacheKey to locate and implement the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0f7c57e3-e734-46f1-ad97-2ec434bba00f
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (5)
packages/plugin-utils/src/resolve.tspackages/plugins/auth-aws/package.jsonpackages/plugins/auth-aws/src/index.tspackages/plugins/auth-aws/src/plugin.test.tspackages/plugins/auth-aws/src/plugin.ts
✅ Files skipped from review due to trivial changes (1)
- packages/plugins/auth-aws/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/plugins/auth-aws/src/index.ts
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/eslint-plugin/src/rules/check-sql.rule.ts (4)
200-212: Minor: Double plugin resolution inapplyPluginDefaults.
applyPluginDefaultscallsresolveConnectionPluginsto get plugins, but callers likecheckConnectionandcheckConnectionByPluginsalso callresolveConnectionPluginsagain afterward. Consider returning the resolved plugins fromapplyPluginDefaultsor refactoring to resolve plugins once at the call site.♻️ Suggested refactor
-function applyPluginDefaults( - connection: RuleOptionConnection, - projectDir: string, -): RuleOptionConnection { - const plugins = resolveConnectionPlugins(connection, projectDir); +function applyPluginDefaults( + connection: RuleOptionConnection, + plugins: SafeQLPlugin[], +): RuleOptionConnection { let merged = connection; for (const plugin of plugins) { if (plugin.connectionDefaults) { merged = deepMergeDefaults(merged, plugin.connectionDefaults) as RuleOptionConnection; } } return merged; }Then in
check:for (let connection of connections) { - connection = applyPluginDefaults(connection, params.projectDir); + const plugins = resolveConnectionPlugins(connection, params.projectDir); + connection = applyPluginDefaults(connection, plugins);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eslint-plugin/src/rules/check-sql.rule.ts` around lines 200 - 212, applyPluginDefaults currently calls resolveConnectionPlugins internally causing duplicate resolution when callers like checkConnection and checkConnectionByPlugins also call resolveConnectionPlugins; refactor applyPluginDefaults to either (A) return both the merged RuleOptionConnection and the resolved plugins (e.g., change signature to return { connection, plugins }) or (B) remove the internal resolveConnectionPlugins call and accept plugins as an extra parameter; update call sites (checkConnection, checkConnectionByPlugins, and any check wrapper) to resolve plugins once and pass them into applyPluginDefaults (or consume the returned plugins) so plugin resolution happens a single time.
360-391: Type casts may warrant runtime validation or type guards.Lines 368 and 374 use type casts (
as PluginResolvedTarget,as ExpectedResolvedTarget) that assume structural compatibility between different target types. While this is likely safe given the shared structure, consider:
- Adding type guards if the structures diverge in the future.
- Documenting the assumption that these types are structurally compatible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eslint-plugin/src/rules/check-sql.rule.ts` around lines 360 - 391, The code unsafely casts result.output to PluginResolvedTarget and target to ExpectedResolvedTarget before passing to params.targetMatch.typeCheck and getResolvedTargetComparableString; add a runtime type guard (e.g., isPluginResolvedTarget / isExpectedResolvedTarget) that validates the minimal shape needed (fields accessed by typeCheck and getResolvedTargetComparableString) and use it to narrow result.output and target before the casts, or change the typeCheck/getResolvedTargetComparableString signatures to accept the broader union; update the block around params.targetMatch.typeCheck, getResolvedTargetComparableString, and report to run the guard and only call these APIs when the guard passes (otherwise handle/report a clear mismatch).
214-235: Consider edge case: shallow copy of nested default objects.On line 229, when
existingis not an object butdefValis, a shallow copy is created. IfdefValcontains nested objects, they remain as shared references. While unlikely to cause issues in the current plugin defaults use case, this could lead to subtle mutation bugs if defaults contain deeply nested mutable structures.If plugin defaults might include deeply nested objects, consider using a recursive deep-clone or a library like
structuredClone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eslint-plugin/src/rules/check-sql.rule.ts` around lines 214 - 235, deepMergeDefaults currently does a shallow copy when defVal is an object but existing is not, which can leave nested objects shared; replace the shallow spread { ...(defVal as Record<string, unknown>) } with a deep clone call so nested structures are copied (e.g. use deepMergeDefaults({}, defVal as Record<string, unknown>) or structuredClone(defVal) if available) in the function deepMergeDefaults to ensure nested default objects are fully cloned instead of referenced.
265-270: Consider documenting the "last plugin wins" behavior.When multiple plugins provide
onTarget()hooks, the current implementation returns the last non-undefined result, meaning later plugins override earlier ones. This may be intentional, but the behavior should be documented for plugin authors.Consider whether this is the desired semantics, or if you want first-match, explicit priority, or chained composition.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/eslint-plugin/src/rules/check-sql.rule.ts` around lines 265 - 270, The loop over plugins currently assigns the last non-undefined onTarget() return to result (symbols: plugins, onTarget, result, tag, targetCtx), so later plugins override earlier ones; update the codebase documentation (e.g., rule README or JSDoc for the function that contains this loop) to explicitly state the "last plugin wins" semantics and how onTarget results are resolved, and additionally add a short comment above the loop explaining this behavior and suggesting alternatives (first-match, priority, or composition) so plugin authors know the intended semantics and where to change it if different behavior is desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/eslint-plugin/src/rules/check-sql.rule.ts`:
- Around line 200-212: applyPluginDefaults currently calls
resolveConnectionPlugins internally causing duplicate resolution when callers
like checkConnection and checkConnectionByPlugins also call
resolveConnectionPlugins; refactor applyPluginDefaults to either (A) return both
the merged RuleOptionConnection and the resolved plugins (e.g., change signature
to return { connection, plugins }) or (B) remove the internal
resolveConnectionPlugins call and accept plugins as an extra parameter; update
call sites (checkConnection, checkConnectionByPlugins, and any check wrapper) to
resolve plugins once and pass them into applyPluginDefaults (or consume the
returned plugins) so plugin resolution happens a single time.
- Around line 360-391: The code unsafely casts result.output to
PluginResolvedTarget and target to ExpectedResolvedTarget before passing to
params.targetMatch.typeCheck and getResolvedTargetComparableString; add a
runtime type guard (e.g., isPluginResolvedTarget / isExpectedResolvedTarget)
that validates the minimal shape needed (fields accessed by typeCheck and
getResolvedTargetComparableString) and use it to narrow result.output and target
before the casts, or change the typeCheck/getResolvedTargetComparableString
signatures to accept the broader union; update the block around
params.targetMatch.typeCheck, getResolvedTargetComparableString, and report to
run the guard and only call these APIs when the guard passes (otherwise
handle/report a clear mismatch).
- Around line 214-235: deepMergeDefaults currently does a shallow copy when
defVal is an object but existing is not, which can leave nested objects shared;
replace the shallow spread { ...(defVal as Record<string, unknown>) } with a
deep clone call so nested structures are copied (e.g. use deepMergeDefaults({},
defVal as Record<string, unknown>) or structuredClone(defVal) if available) in
the function deepMergeDefaults to ensure nested default objects are fully cloned
instead of referenced.
- Around line 265-270: The loop over plugins currently assigns the last
non-undefined onTarget() return to result (symbols: plugins, onTarget, result,
tag, targetCtx), so later plugins override earlier ones; update the codebase
documentation (e.g., rule README or JSDoc for the function that contains this
loop) to explicitly state the "last plugin wins" semantics and how onTarget
results are resolved, and additionally add a short comment above the loop
explaining this behavior and suggesting alternatives (first-match, priority, or
composition) so plugin authors know the intended semantics and where to change
it if different behavior is desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8ae5eab5-f2c3-4b5b-8a3c-37629429660e
📒 Files selected for processing (1)
packages/eslint-plugin/src/rules/check-sql.rule.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/plugin-utils/src/resolve.ts (1)
138-146:⚠️ Potential issue | 🟠 MajorValidate
createConnectionshape during plugin load.The current validation only checks that
plugin.nameis a string. If a plugin returns a malformedcreateConnection(e.g., missingcacheKeyorhandleris not a function), the error will surface later whenconnection-manager.tsinvokeshandler(), making debugging harder.🔧 Proposed fix: Add runtime contract checks for createConnection
const plugin = withFactory(descriptor.config ?? {}) as SafeQLPlugin; if (!plugin || typeof plugin.name !== "string") { throw new Error( `SafeQL plugin "${descriptor.package}" factory must return an object with at least a "name" property.`, ); } + + if ( + plugin.createConnection && + (typeof plugin.createConnection.cacheKey !== "string" || + typeof plugin.createConnection.handler !== "function") + ) { + throw new Error( + `SafeQL plugin "${descriptor.package}" returned invalid "createConnection". Expected { cacheKey: string; handler: () => Promise<Sql> }.`, + ); + } return plugin;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/plugin-utils/src/resolve.ts` around lines 138 - 146, The plugin validation currently only checks plugin.name; extend it to also validate the createConnection contract returned by withFactory(descriptor.config ?? {}) (cast as SafeQLPlugin): ensure if plugin.createConnection exists it is an object with a string cacheKey property and a function handler property (and optionally validate handler signature if feasible), and throw a clear Error referencing descriptor.package and plugin.name when createConnection is missing required fields or has wrong types so malformed handlers are caught at plugin load time before connection-manager invokes handler().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/plugin-utils/src/resolve.ts`:
- Around line 138-146: The plugin validation currently only checks plugin.name;
extend it to also validate the createConnection contract returned by
withFactory(descriptor.config ?? {}) (cast as SafeQLPlugin): ensure if
plugin.createConnection exists it is an object with a string cacheKey property
and a function handler property (and optionally validate handler signature if
feasible), and throw a clear Error referencing descriptor.package and
plugin.name when createConnection is missing required fields or has wrong types
so malformed handlers are caught at plugin load time before connection-manager
invokes handler().
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b17d86c-7d57-483b-8bd3-275cbd17b32a
📒 Files selected for processing (5)
packages/connection-manager/src/index.test.tspackages/connection-manager/src/pg.utils.test.tspackages/connection-manager/src/pg.utils.tspackages/connection-manager/src/test-driver.tspackages/plugin-utils/src/resolve.ts
✅ Files skipped from review due to trivial changes (1)
- packages/connection-manager/src/pg.utils.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/connection-manager/src/pg.utils.ts
Newbie012
force-pushed
the
feat-plugin-system
branch
from
860e60b to
33aa060
Compare
- Refactor auth-aws plugin into separate plugin.ts with tests - Add pg.utils tests for connection manager - Fix zod-annotator test driver to use createProgram() anchored to package root for reliable TypeScript lib resolution in CI - Add _type property tests for zod annotator - Fix resolve.ts to handle edge cases
Newbie012
force-pushed
the
feat-plugin-system
branch
from
33aa060 to
b3cc343
Compare
This was referenced Mar 21, 2026
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.
What changed
@ts-safeql/plugin-auth-awsfor AWS RDS IAM authenticationWhy
Enables modular extensibility—users can add plugins for custom authentication, connection logic, and future hooks without hardcoding every scenario. Keeps SafeQL’s worker-based design while making it extensible.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores