chore(core): Add support for persisting and synchronizing credential overwrites #2
Conversation
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
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 12 files
Prompt for AI agents (all 11 issues)
Understand the root cause of the following 11 issues and fix them.
<file name="packages/cli/src/commands/start.ts">
<violation number="1" location="packages/cli/src/commands/start.ts:36">
Import from `@/credentials-overwrites` is placed after a relative import, breaking the file's established import grouping/order and likely violating import-order linting.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Stylistic import-order deviation with no functional impact; lint enforcement in this package is unclear. Low importance—filter out.</violation>
<violation number="2" location="packages/cli/src/commands/start.ts:231">
Unhandled parse error in CredentialsOverwrites.init for env-provided overwrites causes startup crash.</violation>
</file>
<file name="packages/@n8n/decorators/src/pubsub/pubsub-metadata.ts">
<violation number="1" location="packages/@n8n/decorators/src/pubsub/pubsub-metadata.ts:21">
Event name is likely inconsistent with established naming and the feature’s noun phrase. Prefer 'reload-credential-overwrites' to match 'credential overwrites' and existing 'reload-*' patterns.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: uncertaintyLanguage.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Consistent usage across the codebase and tests; purely stylistic naming preference with no functional impact.</violation>
</file>
<file name="packages/cli/src/__tests__/credentials-overwrites.test.ts">
<violation number="1" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:722">
Test name claims graceful handling but the assertion expects a thrown error, causing misleading test intent.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Title wording is subjective and lower impact; redundant with clearer comment/assertion mismatch in Violation #1.
• **Libraries consulted**: Jest doMock ESM dynamic import behavior, Jest</violation>
<violation number="2" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:744">
jest.doMock called after the module was already imported in this suite; dynamic import may return the cached module, so the mock throwing factory might not take effect, causing flaky tests.</violation>
<violation number="3" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:755">
Inline comment contradicts the assertion that expects a rejection.</violation>
</file>
<file name="packages/cli/src/scaling/pubsub/pubsub.event-map.ts">
<violation number="1" location="packages/cli/src/scaling/pubsub/pubsub.event-map.ts:15">
Use consistent region comment style: replace `// # region Credentials` with `// #region Credentials` for consistency within the file.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Incorrect and trivial: both comment styles exist in the file; even if inconsistent, it’s a minor cosmetic issue with no impact.</violation>
</file>
<file name="packages/@n8n/config/src/configs/credentials.config.ts">
<violation number="1" location="packages/@n8n/config/src/configs/credentials.config.ts:20">
Misleading JSDoc: description refers to auth token but this flag controls persistence of credential overwrites. Update the comment to accurately describe the `persistence` setting.</violation>
</file>
<file name="packages/cli/src/scaling/pubsub/pubsub.types.ts">
<violation number="1" location="packages/cli/src/scaling/pubsub/pubsub.types.ts:45">
Type alias name uses “Overrides” but the feature and command use “overwrites”. Rename to “ReloadCredentialsOverwrites” for consistency with the domain term and command key.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*</violation>
</file>
<file name="packages/cli/src/server.ts">
<violation number="1" location="packages/cli/src/server.ts:300">
Authentication enforcement is misdetected using `!== null`, which treats `undefined` as authenticated and allows repeated overwrites without auth. Use a truthiness check to correctly reflect middleware presence.</violation>
<violation number="2" location="packages/cli/src/server.ts:318">
Awaited `setData(body, true)` has no error handling in this async route; a rejection may result in an unhandled promise or inconsistent error response. Wrap in try/catch or handle the promise explicitly with `ResponseHelper`.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| Container.get(WaitTracker).init(); | ||
| this.logger.debug('Wait tracker init complete'); | ||
| await Container.get(CredentialsOverwrites).init(); |
There was a problem hiding this comment.
Unhandled parse error in CredentialsOverwrites.init for env-provided overwrites causes startup crash.
Prompt for AI agents
Address the following comment on packages/cli/src/commands/start.ts at line 231:
<comment>Unhandled parse error in CredentialsOverwrites.init for env-provided overwrites causes startup crash.</comment>
<file context>
@@ -227,6 +228,8 @@ export class Start extends BaseCommand<z.infer<typeof flagsSchema>> {
Container.get(WaitTracker).init();
this.logger.debug('Wait tracker init complete');
+ await Container.get(CredentialsOverwrites).init();
+ this.logger.debug('Credentials overwrites init complete');
await this.initBinaryDataService();
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Functional Bugs Agent
| import { WorkflowRunner } from '@/workflow-runner'; | ||
|
|
||
| import { BaseCommand } from './base-command'; | ||
| import { CredentialsOverwrites } from '@/credentials-overwrites'; |
There was a problem hiding this comment.
Import from @/credentials-overwrites is placed after a relative import, breaking the file's established import grouping/order and likely violating import-order linting.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Stylistic import-order deviation with no functional impact; lint enforcement in this package is unclear. Low importance—filter out.
Prompt for AI agents
Address the following comment on packages/cli/src/commands/start.ts at line 36:
<comment>Import from `@/credentials-overwrites` is placed after a relative import, breaking the file's established import grouping/order and likely violating import-order linting.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Stylistic import-order deviation with no functional impact; lint enforcement in this package is unclear. Low importance—filter out.</comment>
<file context>
@@ -33,6 +33,7 @@ import { WaitTracker } from '@/wait-tracker';
import { WorkflowRunner } from '@/workflow-runner';
import { BaseCommand } from './base-command';
+import { CredentialsOverwrites } from '@/credentials-overwrites';
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
| | 'reload-license' | ||
| | 'reload-oidc-config' | ||
| | 'reload-saml-config' | ||
| | 'reload-overwrite-credentials' |
There was a problem hiding this comment.
Event name is likely inconsistent with established naming and the feature’s noun phrase. Prefer 'reload-credential-overwrites' to match 'credential overwrites' and existing 'reload-*' patterns.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: uncertaintyLanguage.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Consistent usage across the codebase and tests; purely stylistic naming preference with no functional impact.
Prompt for AI agents
Address the following comment on packages/@n8n/decorators/src/pubsub/pubsub-metadata.ts at line 21:
<comment>Event name is likely inconsistent with established naming and the feature’s noun phrase. Prefer 'reload-credential-overwrites' to match 'credential overwrites' and existing 'reload-*' patterns.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: uncertaintyLanguage.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Consistent usage across the codebase and tests; purely stylistic naming preference with no functional impact.</comment>
<file context>
@@ -18,6 +18,7 @@ export type PubSubEventName =
| 'reload-license'
| 'reload-oidc-config'
| 'reload-saml-config'
+ | 'reload-overwrite-credentials'
| 'response-to-get-worker-status'
| 'restart-event-bus'
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: General AI Review Agent
| ); | ||
|
|
||
| // Mock a module import failure | ||
| jest.doMock('@/scaling/pubsub/publisher.service', () => { |
There was a problem hiding this comment.
jest.doMock called after the module was already imported in this suite; dynamic import may return the cached module, so the mock throwing factory might not take effect, causing flaky tests.
Prompt for AI agents
Address the following comment on packages/cli/src/__tests__/credentials-overwrites.test.ts at line 744:
<comment>jest.doMock called after the module was already imported in this suite; dynamic import may return the cached module, so the mock throwing factory might not take effect, causing flaky tests.</comment>
<file context>
@@ -234,4 +254,963 @@ describe('CredentialsOverwrites', () => {
+ );
+
+ // Mock a module import failure
+ jest.doMock('@/scaling/pubsub/publisher.service', () => {
+ throw new Error('Publisher service not available');
+ });
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
| loadOnStartup: false, | ||
| }); | ||
|
|
||
| // Should not throw error even if Publisher import fails |
There was a problem hiding this comment.
Inline comment contradicts the assertion that expects a rejection.
Prompt for AI agents
Address the following comment on packages/cli/src/__tests__/credentials-overwrites.test.ts at line 755:
<comment>Inline comment contradicts the assertion that expects a rejection.</comment>
<file context>
@@ -234,4 +254,963 @@ describe('CredentialsOverwrites', () => {
+ loadOnStartup: false,
+ });
+
+ // Should not throw error even if Publisher import fails
+ await expect(
+ errorCredentialsOverwrites.saveOverwriteDataToDB({ test: { username: 'user' } }, true),
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: General AI Review Agent
Suggested change
| // Should not throw error even if Publisher import fails | |
| // Should throw error when Publisher import fails |
|
|
||
| // #endregion | ||
|
|
||
| // # region Credentials |
There was a problem hiding this comment.
Use consistent region comment style: replace // # region Credentials with // #region Credentials for consistency within the file.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Incorrect and trivial: both comment styles exist in the file; even if inconsistent, it’s a minor cosmetic issue with no impact.
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/pubsub/pubsub.event-map.ts at line 15:
<comment>Use consistent region comment style: replace `// # region Credentials` with `// #region Credentials` for consistency within the file.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Incorrect and trivial: both comment styles exist in the file; even if inconsistent, it’s a minor cosmetic issue with no impact.</comment>
<file context>
@@ -12,6 +12,10 @@ export type PubSubCommandMap = {
// #endregion
+ // # region Credentials
+ 'reload-overwrite-credentials': never;
+ // #endregion
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
| @Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN') | ||
| endpointAuthToken: string = ''; | ||
|
|
||
| /** Authentication token for the credentials overwrite endpoint. */ |
There was a problem hiding this comment.
Misleading JSDoc: description refers to auth token but this flag controls persistence of credential overwrites. Update the comment to accurately describe the persistence setting.
Prompt for AI agents
Address the following comment on packages/@n8n/config/src/configs/credentials.config.ts at line 20:
<comment>Misleading JSDoc: description refers to auth token but this flag controls persistence of credential overwrites. Update the comment to accurately describe the `persistence` setting.</comment>
<file context>
@@ -16,6 +16,10 @@ class CredentialsOverwrite {
@Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN')
endpointAuthToken: string = '';
+
+ /** Authentication token for the credentials overwrite endpoint. */
+ @Env('CREDENTIALS_OVERWRITE_PERSISTENCE')
+ persistence: boolean = false;
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: General AI Review Agent
| export type ReloadLicense = ToCommand<'reload-license'>; | ||
| export type ReloadOIDCConfiguration = ToCommand<'reload-oidc-config'>; | ||
| export type ReloadSamlConfiguration = ToCommand<'reload-saml-config'>; | ||
| export type ReloadCredentialsOverrides = ToCommand<'reload-overwrite-credentials'>; |
There was a problem hiding this comment.
Type alias name uses “Overrides” but the feature and command use “overwrites”. Rename to “ReloadCredentialsOverwrites” for consistency with the domain term and command key.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/pubsub/pubsub.types.ts at line 45:
<comment>Type alias name uses “Overrides” but the feature and command use “overwrites”. Rename to “ReloadCredentialsOverwrites” for consistency with the domain term and command key.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*</comment>
<file context>
@@ -42,6 +42,7 @@ export namespace PubSub {
export type ReloadLicense = ToCommand<'reload-license'>;
export type ReloadOIDCConfiguration = ToCommand<'reload-oidc-config'>;
export type ReloadSamlConfiguration = ToCommand<'reload-saml-config'>;
+ export type ReloadCredentialsOverrides = ToCommand<'reload-overwrite-credentials'>;
export type RestartEventBus = ToCommand<'restart-event-bus'>;
export type ReloadExternalSecretsProviders = ToCommand<'reload-external-secrets-providers'>;
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
| } | ||
|
|
||
| Container.get(CredentialsOverwrites).setData(body); | ||
| await Container.get(CredentialsOverwrites).setData(body, true); |
There was a problem hiding this comment.
Awaited setData(body, true) has no error handling in this async route; a rejection may result in an unhandled promise or inconsistent error response. Wrap in try/catch or handle the promise explicitly with ResponseHelper.
Prompt for AI agents
Address the following comment on packages/cli/src/server.ts at line 318:
<comment>Awaited `setData(body, true)` has no error handling in this async route; a rejection may result in an unhandled promise or inconsistent error response. Wrap in try/catch or handle the promise explicitly with `ResponseHelper`.</comment>
<file context>
@@ -313,7 +315,7 @@ export class Server extends AbstractServer {
}
- Container.get(CredentialsOverwrites).setData(body);
+ await Container.get(CredentialsOverwrites).setData(body, true);
await frontendService?.generateTypes();
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
| this.app.use(`/${this.endpointPresetCredentials}`, overwriteEndpointMiddleware); | ||
| } | ||
|
|
||
| const authenticationEnforced = overwriteEndpointMiddleware !== null; |
There was a problem hiding this comment.
Authentication enforcement is misdetected using !== null, which treats undefined as authenticated and allows repeated overwrites without auth. Use a truthiness check to correctly reflect middleware presence.
Prompt for AI agents
Address the following comment on packages/cli/src/server.ts at line 300:
<comment>Authentication enforcement is misdetected using `!== null`, which treats `undefined` as authenticated and allows repeated overwrites without auth. Use a truthiness check to correctly reflect middleware presence.</comment>
<file context>
@@ -297,10 +297,12 @@ export class Server extends AbstractServer {
this.app.use(`/${this.endpointPresetCredentials}`, overwriteEndpointMiddleware);
}
+ const authenticationEnforced = overwriteEndpointMiddleware !== null;
this.app.post(
`/${this.endpointPresetCredentials}`,
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
![cubic-staging[bot]](https://avatars.githubusercontent.com/u/189440881?s=60&v=4){kind=small}
There was a problem hiding this comment.
30 issues found across 12 files
Prompt for AI agents (all 30 issues)
Understand the root cause of the following 30 issues and fix them.
<file name="packages/@n8n/config/src/configs/credentials.config.ts">
<violation number="1" location="packages/@n8n/config/src/configs/credentials.config.ts:20">
two-agent-filter: Incorrect/misleading doc comment for `persistence`; should describe enabling persistence of credential overwrites.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Accurate but trivial doc comment mismatch with no functional impact; not worth reporting per criteria.</violation>
<violation number="2" location="packages/@n8n/config/src/configs/credentials.config.ts:20">
single-agent-filter: Incorrect/misleading doc comment for `persistence`; should describe enabling persistence of credential overwrites.
• **Libraries consulted**:</violation>
</file>
<file name="packages/cli/src/scaling/worker-server.ts">
<violation number="1" location="packages/cli/src/scaling/worker-server.ts:124">
two-agent-filter: Credentials overwrite processing logic duplicates packages/cli/src/server.ts's app.post handler. The core logic for handling JSON body validation, calling `credentialsOverwrites.setData(..., true)`, setting a loaded flag, and sending a success response is nearly identical. This can lead to maintenance challenges and inconsistencies if business logic changes. Consider abstracting this common processing into a shared utility or method within `CredentialsOverwrites` or a dedicated handler.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Not high-impact duplication. Only minor shared steps (content-type check + setData). server.ts adds auth enforcement and generateTypes, with different errors/flags—distinct responsibilities; unification unwarranted.</violation>
<violation number="2" location="packages/cli/src/scaling/worker-server.ts:124">
single-agent-filter: Credentials overwrite processing logic duplicates packages/cli/src/server.ts's app.post handler. The core logic for handling JSON body validation, calling `credentialsOverwrites.setData(..., true)`, setting a loaded flag, and sending a success response is nearly identical. This can lead to maintenance challenges and inconsistencies if business logic changes. Consider abstracting this common processing into a shared utility or method within `CredentialsOverwrites` or a dedicated handler.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Filter: The two handlers live in different contexts (main server vs worker) and have important differences (auth-enforced repeat submissions in server.ts, generateTypes call, different error handling, different loaded flags). The overlapping steps (content-type check + setData + success response) are minimal and not high-impact duplication. Unifying them could couple distinct flows without clear benefit.</violation>
<violation number="3" location="packages/cli/src/scaling/worker-server.ts:159">
two-agent-filter: Race condition: awaiting setData() before setting the overwritesLoaded flag allows concurrent POSTs to bypass the guard and perform duplicate writes/broadcasts. Consider setting the flag before the await and handling rollback on failure.
• **Libraries consulted**:</violation>
<violation number="4" location="packages/cli/src/scaling/worker-server.ts:159">
single-agent-filter: Race condition: awaiting setData() before setting the overwritesLoaded flag allows concurrent POSTs to bypass the guard and perform duplicate writes/broadcasts. Consider setting the flag before the await and handling rollback on failure.
• **Libraries consulted**:</violation>
</file>
<file name="packages/cli/src/scaling/pubsub/pubsub.event-map.ts">
<violation number="1" location="packages/cli/src/scaling/pubsub/pubsub.event-map.ts:15">
two-agent-filter: Inconsistent region comment style: use `// #region Credentials` (no space) to match the predominant style in this file.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact, documentationImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Stylistic-only; file already mixes both region styles (e.g., SSO uses spaced). No functional or maintainability impact.</violation>
<violation number="2" location="packages/cli/src/scaling/pubsub/pubsub.event-map.ts:15">
single-agent-filter: Inconsistent region comment style: use `// #region Credentials` (no space) to match the predominant style in this file.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact, documentationImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Trivial stylistic inconsistency in comment spacing; no functional impact and negligible maintainability value. The file already mixes both styles. Not worth reporting.</violation>
</file>
<file name="packages/cli/src/scaling/pubsub/pubsub.types.ts">
<violation number="1" location="packages/cli/src/scaling/pubsub/pubsub.types.ts:45">
two-agent-filter: Naming inconsistency: use “Overwrites” instead of “Overrides” to align with command key `reload-overwrite-credentials` and existing `CredentialsOverwrites` naming.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Purely a naming inconsistency (Overrides vs Overwrites). Command key is correct and functionality/types are unaffected. Low impact and stylistic; not worth reporting.</violation>
<violation number="2" location="packages/cli/src/scaling/pubsub/pubsub.types.ts:45">
single-agent-filter: Naming inconsistency: use “Overwrites” instead of “Overrides” to align with command key `reload-overwrite-credentials` and existing `CredentialsOverwrites` naming.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Technically correct but purely a naming inconsistency with negligible functional impact. No behavior, typing, or API issues; fits existing command-type naming pattern and does not affect stability or maintainability materially. Filter as low severity/style.</violation>
</file>
<file name="packages/cli/src/server.ts">
<violation number="1" location="packages/cli/src/server.ts:305">
two-agent-filter: API endpoint handler logic for setting preset credentials duplicates the `handleOverwrites()` method in `packages/cli/src/scaling/worker-server.ts`. Both handle content type validation, call `CredentialsOverwrites.setData()`, and manage a loaded flag with success/error responses. The only significant difference is the `authenticationEnforced` check in this file's conditional.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*
• **Libraries consulted**:</violation>
<violation number="2" location="packages/cli/src/server.ts:305">
single-agent-filter: API endpoint handler logic for setting preset credentials duplicates the `handleOverwrites()` method in `packages/cli/src/scaling/worker-server.ts`. Both handle content type validation, call `CredentialsOverwrites.setData()`, and manage a loaded flag with success/error responses. The only significant difference is the `authenticationEnforced` check in this file's conditional.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*
• **Libraries consulted**:</violation>
<violation number="3" location="packages/cli/src/server.ts:318">
two-agent-filter: Calling setData with default reloadFrontend=true already regenerates UI types; the next call to generateTypes duplicates work. Pass false for reloadFrontend to avoid double regeneration.
• **Libraries consulted**:</violation>
<violation number="4" location="packages/cli/src/server.ts:318">
single-agent-filter: Calling setData with default reloadFrontend=true already regenerates UI types; the next call to generateTypes duplicates work. Pass false for reloadFrontend to avoid double regeneration.
• **Libraries consulted**:</violation>
</file>
<file name="packages/cli/src/__tests__/credentials-overwrites.test.ts">
<violation number="1" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:722">
two-agent-filter: Test name claims graceful handling but the assertion expects a thrown error. Align the test name with the actual expectation to avoid confusion.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Minor naming inconsistency; low impact and no functional effect.</violation>
<violation number="2" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:722">
single-agent-filter: Test name claims graceful handling but the assertion expects a thrown error. Align the test name with the actual expectation to avoid confusion.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: This is a naming/style issue; “gracefully” is subjective and does not affect test correctness. Low impact and not worth reporting.
• **Libraries consulted**: Jest mocking modules jest.doMock behavior after module import, Jestjs_io-docs-getting-started</violation>
<violation number="3" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:744">
two-agent-filter: jest.doMock is applied after the module was already imported and cached, so the dynamic import will use the cached module and not the mock; this test won’t reliably simulate an import failure.
• **Libraries consulted**:</violation>
<violation number="4" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:744">
single-agent-filter: jest.doMock is applied after the module was already imported and cached, so the dynamic import will use the cached module and not the mock; this test won’t reliably simulate an import failure.
• **Libraries consulted**: Jest mocking modules jest.doMock behavior after module import, Jestjs_io-docs-getting-started</violation>
<violation number="5" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:755">
two-agent-filter: Inline comment contradicts the assertion below it; update the comment to reflect the expected behavior.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Comment/assertion mismatch is documentation-only with no functional impact.</violation>
<violation number="6" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:755">
single-agent-filter: Inline comment contradicts the assertion below it; update the comment to reflect the expected behavior.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Minor comment mismatch with the assertion. It’s low-impact documentation noise and doesn’t affect functionality; deprioritize per selectivity criteria.
• **Libraries consulted**: Jest mocking modules jest.doMock behavior after module import, Jestjs_io-docs-getting-started</violation>
</file>
<file name="packages/cli/src/credentials-overwrites.ts">
<violation number="1" location="packages/cli/src/credentials-overwrites.ts:57">
two-agent-filter: Early return on concurrent reload lacks logging; consider adding a debug log for visibility.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Minor logging preference with no functional impact.</violation>
<violation number="2" location="packages/cli/src/credentials-overwrites.ts:57">
single-agent-filter: Early return on concurrent reload lacks logging; consider adding a debug log for visibility.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Missing log on early return is a minor style/observability nit with negligible functional impact.</violation>
<violation number="3" location="packages/cli/src/credentials-overwrites.ts:91">
two-agent-filter: Setting loadOnStartup to false may conflict with desired auto-load behavior; verify whether this setting should be true for credential overwrites.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: uncertaintyLanguage.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: loadOnStartup: false matches explicit DB loading flow; no evidence of incorrect behavior.</violation>
<violation number="4" location="packages/cli/src/credentials-overwrites.ts:91">
single-agent-filter: Setting loadOnStartup to false may conflict with desired auto-load behavior; verify whether this setting should be true for credential overwrites.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: uncertaintyLanguage.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Speculative. The service handles loading from DB explicitly; setting loadOnStartup: false aligns with controlled loading. No evidence of conflict.</violation>
<violation number="5" location="packages/cli/src/credentials-overwrites.ts:145">
two-agent-filter: This change introduces a circular dependency between `CredentialsOverwrites` and `FrontendService` by using a service locator (Container.get), which couples responsibilities and violates SRP; use an event or dependency-injection pattern instead.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Evidence does not clearly match this PR; code uses dynamic import and DI to avoid static cycles. No demonstrated functional issue; reads as an architectural preference, not a high-impact bug.</violation>
<violation number="6" location="packages/cli/src/credentials-overwrites.ts:145">
single-agent-filter: This change introduces a circular dependency between `CredentialsOverwrites` and `FrontendService` by using a service locator (Container.get), which couples responsibilities and violates SRP; use an event or dependency-injection pattern instead.
• **Libraries consulted**:</violation>
<violation number="7" location="packages/cli/src/credentials-overwrites.ts:149">
two-agent-filter: Container.get may throw if FrontendService is not registered; optional chaining here does not guard against that. Consider checking registration before get or wrap in try/catch.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: FrontendService is registered and imported before use; Container.get will return an instance. Optional chaining is redundant, not a defect.</violation>
<violation number="8" location="packages/cli/src/credentials-overwrites.ts:149">
single-agent-filter: Container.get may throw if FrontendService is not registered; optional chaining here does not guard against that. Consider checking registration before get or wrap in try/catch.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: FrontendService is decorated with @Service and dynamically imported, ensuring registration before Container.get. DI get throws only if not decorated; here it won’t. Optional chaining is redundant but harmless; not a high-impact issue.</violation>
</file>
<file name="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts">
<violation number="1" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:273">
two-agent-filter: This test claims to handle database failures but passes storeInDb=false, so no DB operation occurs. The failure path isn't exercised and resilience is not validated.
• **Libraries consulted**:</violation>
<violation number="2" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:273">
single-agent-filter: This test claims to handle database failures but passes storeInDb=false, so no DB operation occurs. The failure path isn't exercised and resilience is not validated.
• **Libraries consulted**:</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| @Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN') | ||
| endpointAuthToken: string = ''; | ||
|
|
||
| /** Authentication token for the credentials overwrite endpoint. */ |
There was a problem hiding this comment.
two-agent-filter: Incorrect/misleading doc comment for persistence; should describe enabling persistence of credential overwrites.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Accurate but trivial doc comment mismatch with no functional impact; not worth reporting per criteria.
Prompt for AI agents
Address the following comment on packages/@n8n/config/src/configs/credentials.config.ts at line 20:
<comment>two-agent-filter: Incorrect/misleading doc comment for `persistence`; should describe enabling persistence of credential overwrites.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Accurate but trivial doc comment mismatch with no functional impact; not worth reporting per criteria.</comment>
<file context>
@@ -16,6 +16,10 @@ class CredentialsOverwrite {
@Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN')
endpointAuthToken: string = '';
+
+ /** Authentication token for the credentials overwrite endpoint. */
+ @Env('CREDENTIALS_OVERWRITE_PERSISTENCE')
+ persistence: boolean = false;
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
Suggested change
| /** Authentication token for the credentials overwrite endpoint. */ | |
| /** Whether to persist credential overwrites in the database. */ |
| @Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN') | ||
| endpointAuthToken: string = ''; | ||
|
|
||
| /** Authentication token for the credentials overwrite endpoint. */ |
There was a problem hiding this comment.
single-agent-filter: Incorrect/misleading doc comment for persistence; should describe enabling persistence of credential overwrites.
• Libraries consulted:
Prompt for AI agents
Address the following comment on packages/@n8n/config/src/configs/credentials.config.ts at line 20:
<comment>single-agent-filter: Incorrect/misleading doc comment for `persistence`; should describe enabling persistence of credential overwrites.
• **Libraries consulted**: </comment>
<file context>
@@ -16,6 +16,10 @@ class CredentialsOverwrite {
@Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN')
endpointAuthToken: string = '';
+
+ /** Authentication token for the credentials overwrite endpoint. */
+ @Env('CREDENTIALS_OVERWRITE_PERSISTENCE')
+ persistence: boolean = false;
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
Suggested change
| /** Authentication token for the credentials overwrite endpoint. */ | |
| /** Whether to persist credential overwrites in the database. */ |
| this.handleOverwrites(req, res), | ||
| ); | ||
| this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, async (req, res) => { | ||
| await this.handleOverwrites(req, res); |
There was a problem hiding this comment.
two-agent-filter: Credentials overwrite processing logic duplicates packages/cli/src/server.ts's app.post handler. The core logic for handling JSON body validation, calling credentialsOverwrites.setData(..., true), setting a loaded flag, and sending a success response is nearly identical. This can lead to maintenance challenges and inconsistencies if business logic changes. Consider abstracting this common processing into a shared utility or method within CredentialsOverwrites or a dedicated handler.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Not high-impact duplication. Only minor shared steps (content-type check + setData). server.ts adds auth enforcement and generateTypes, with different errors/flags—distinct responsibilities; unification unwarranted.
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/worker-server.ts at line 124:
<comment>two-agent-filter: Credentials overwrite processing logic duplicates packages/cli/src/server.ts's app.post handler. The core logic for handling JSON body validation, calling `credentialsOverwrites.setData(..., true)`, setting a loaded flag, and sending a success response is nearly identical. This can lead to maintenance challenges and inconsistencies if business logic changes. Consider abstracting this common processing into a shared utility or method within `CredentialsOverwrites` or a dedicated handler.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Not high-impact duplication. Only minor shared steps (content-type check + setData). server.ts adds auth enforcement and generateTypes, with different errors/flags—distinct responsibilities; unification unwarranted.</comment>
<file context>
@@ -120,9 +120,9 @@ export class WorkerServer {
- this.handleOverwrites(req, res),
- );
+ this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, async (req, res) => {
+ await this.handleOverwrites(req, res);
+ });
}
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Unmapped Agent in getAgentNameFromViolationSource (undefined)
| this.handleOverwrites(req, res), | ||
| ); | ||
| this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, async (req, res) => { | ||
| await this.handleOverwrites(req, res); |
There was a problem hiding this comment.
single-agent-filter: Credentials overwrite processing logic duplicates packages/cli/src/server.ts's app.post handler. The core logic for handling JSON body validation, calling credentialsOverwrites.setData(..., true), setting a loaded flag, and sending a success response is nearly identical. This can lead to maintenance challenges and inconsistencies if business logic changes. Consider abstracting this common processing into a shared utility or method within CredentialsOverwrites or a dedicated handler.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Filter: The two handlers live in different contexts (main server vs worker) and have important differences (auth-enforced repeat submissions in server.ts, generateTypes call, different error handling, different loaded flags). The overlapping steps (content-type check + setData + success response) are minimal and not high-impact duplication. Unifying them could couple distinct flows without clear benefit.
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/worker-server.ts at line 124:
<comment>single-agent-filter: Credentials overwrite processing logic duplicates packages/cli/src/server.ts's app.post handler. The core logic for handling JSON body validation, calling `credentialsOverwrites.setData(..., true)`, setting a loaded flag, and sending a success response is nearly identical. This can lead to maintenance challenges and inconsistencies if business logic changes. Consider abstracting this common processing into a shared utility or method within `CredentialsOverwrites` or a dedicated handler.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Filter: The two handlers live in different contexts (main server vs worker) and have important differences (auth-enforced repeat submissions in server.ts, generateTypes call, different error handling, different loaded flags). The overlapping steps (content-type check + setData + success response) are minimal and not high-impact duplication. Unifying them could couple distinct flows without clear benefit.</comment>
<file context>
@@ -120,9 +120,9 @@ export class WorkerServer {
- this.handleOverwrites(req, res),
- );
+ this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, async (req, res) => {
+ await this.handleOverwrites(req, res);
+ });
}
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Unmapped Agent in getAgentNameFromViolationSource (undefined)
|
|
||
| // #endregion | ||
|
|
||
| // # region Credentials |
There was a problem hiding this comment.
two-agent-filter: Inconsistent region comment style: use // #region Credentials (no space) to match the predominant style in this file.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact, documentationImpact.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Stylistic-only; file already mixes both region styles (e.g., SSO uses spaced). No functional or maintainability impact.
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/pubsub/pubsub.event-map.ts at line 15:
<comment>two-agent-filter: Inconsistent region comment style: use `// #region Credentials` (no space) to match the predominant style in this file.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact, documentationImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Stylistic-only; file already mixes both region styles (e.g., SSO uses spaced). No functional or maintainability impact.</comment>
<file context>
@@ -12,6 +12,10 @@ export type PubSubCommandMap = {
// #endregion
+ // # region Credentials
+ 'reload-overwrite-credentials': never;
+ // #endregion
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
| const setting = this.settings.create({ | ||
| key: CREDENTIALS_OVERWRITE_KEY, | ||
| value: data, | ||
| loadOnStartup: false, |
There was a problem hiding this comment.
single-agent-filter: Setting loadOnStartup to false may conflict with desired auto-load behavior; verify whether this setting should be true for credential overwrites.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: uncertaintyLanguage.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Speculative. The service handles loading from DB explicitly; setting loadOnStartup: false aligns with controlled loading. No evidence of conflict.
Prompt for AI agents
Address the following comment on packages/cli/src/credentials-overwrites.ts at line 91:
<comment>single-agent-filter: Setting loadOnStartup to false may conflict with desired auto-load behavior; verify whether this setting should be true for credential overwrites.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: uncertaintyLanguage.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Speculative. The service handles loading from DB explicitly; setting loadOnStartup: false aligns with controlled loading. No evidence of conflict.</comment>
<file context>
@@ -18,13 +23,78 @@ export class CredentialsOverwrites {
+ const setting = this.settings.create({
+ key: CREDENTIALS_OVERWRITE_KEY,
+ value: data,
+ loadOnStartup: false,
});
+ await this.settings.save(setting);
</file context>
[internal] Confidence score: 5/10
[internal] Posted by: General AI Review Agent
| } | ||
|
|
||
| async loadOverwriteDataFromDB(reloadFrontend: boolean) { | ||
| if (this.reloading) return; |
There was a problem hiding this comment.
single-agent-filter: Early return on concurrent reload lacks logging; consider adding a debug log for visibility.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Missing log on early return is a minor style/observability nit with negligible functional impact.
Prompt for AI agents
Address the following comment on packages/cli/src/credentials-overwrites.ts at line 57:
<comment>single-agent-filter: Early return on concurrent reload lacks logging; consider adding a debug log for visibility.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Missing log on early return is a minor style/observability nit with negligible functional impact.</comment>
<file context>
@@ -18,13 +23,78 @@ export class CredentialsOverwrites {
+ }
+
+ async loadOverwriteDataFromDB(reloadFrontend: boolean) {
+ if (this.reloading) return;
+ try {
+ this.reloading = true;
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: General AI Review Agent
Suggested change
| if (this.reloading) return; | |
| if (this.reloading) { this.logger.debug('Skipping overwrite reload: already in progress'); return; } |
| // FrontendService has CredentialOverwrites injected via the constructor | ||
| // to break the circular dependency we need to use the container to get the instance | ||
| const { FrontendService } = await import('./services/frontend.service'); | ||
| await Container.get(FrontendService)?.generateTypes(); |
There was a problem hiding this comment.
single-agent-filter: Container.get may throw if FrontendService is not registered; optional chaining here does not guard against that. Consider checking registration before get or wrap in try/catch.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: FrontendService is decorated with @service and dynamically imported, ensuring registration before Container.get. DI get throws only if not decorated; here it won’t. Optional chaining is redundant but harmless; not a high-impact issue.
Prompt for AI agents
Address the following comment on packages/cli/src/credentials-overwrites.ts at line 149:
<comment>single-agent-filter: Container.get may throw if FrontendService is not registered; optional chaining here does not guard against that. Consider checking registration before get or wrap in try/catch.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: FrontendService is decorated with @Service and dynamically imported, ensuring registration before Container.get. DI get throws only if not decorated; here it won’t. Optional chaining is redundant but harmless; not a high-impact issue.</comment>
<file context>
@@ -58,6 +132,21 @@ export class CredentialsOverwrites {
+ // FrontendService has CredentialOverwrites injected via the constructor
+ // to break the circular dependency we need to use the container to get the instance
+ const { FrontendService } = await import('./services/frontend.service');
+ await Container.get(FrontendService)?.generateTypes();
}
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: General AI Review Agent
| ); | ||
|
|
||
| // This should not throw, memory operation should still work | ||
| await failingInstance.setData(testData, false, false); // Don't store in DB |
There was a problem hiding this comment.
two-agent-filter: This test claims to handle database failures but passes storeInDb=false, so no DB operation occurs. The failure path isn't exercised and resilience is not validated.
• Libraries consulted:
Prompt for AI agents
Address the following comment on packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts at line 273:
<comment>two-agent-filter: This test claims to handle database failures but passes storeInDb=false, so no DB operation occurs. The failure path isn't exercised and resilience is not validated.
• **Libraries consulted**: </comment>
<file context>
@@ -0,0 +1,503 @@
+ );
+
+ // This should not throw, memory operation should still work
+ await failingInstance.setData(testData, false, false); // Don't store in DB
+
+ // Memory should still contain the data
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
| ); | ||
|
|
||
| // This should not throw, memory operation should still work | ||
| await failingInstance.setData(testData, false, false); // Don't store in DB |
There was a problem hiding this comment.
single-agent-filter: This test claims to handle database failures but passes storeInDb=false, so no DB operation occurs. The failure path isn't exercised and resilience is not validated.
• Libraries consulted:
Prompt for AI agents
Address the following comment on packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts at line 273:
<comment>single-agent-filter: This test claims to handle database failures but passes storeInDb=false, so no DB operation occurs. The failure path isn't exercised and resilience is not validated.
• **Libraries consulted**: </comment>
<file context>
@@ -0,0 +1,503 @@
+ );
+
+ // This should not throw, memory operation should still work
+ await failingInstance.setData(testData, false, false); // Don't store in DB
+
+ // Memory should still contain the data
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
|
@cubic-dev-local review this |
@sanxroz I've started the AI code review. It'll take a few minutes to complete. |
There was a problem hiding this comment.
46 issues found across 12 files
Prompt for AI agents (all 46 issues)
Understand the root cause of the following 46 issues and fix them.
<file name="packages/cli/src/commands/start.ts">
<violation number="1" location="packages/cli/src/commands/start.ts:36">
single-agent-filter: Import from '@/credentials-overwrites' is placed after a relative import, breaking the existing alias-vs-relative import grouping. Consider moving it to the block with other '@/…' imports for consistency.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact, objectivity.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Technically accurate (alias import placed after a relative import), but purely stylistic with no functional impact. This is a low-severity consistency nit that should be handled by linting/formatting rules if desired, not a high-impact issue worth flagging.</violation>
<violation number="2" location="packages/cli/src/commands/start.ts:36">
two-agent-filter: Import from '@/credentials-overwrites' is placed after a relative import, breaking the existing alias-vs-relative import grouping. Consider moving it to the block with other '@/…' imports for consistency.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact, objectivity.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Accurate but purely stylistic import-order grouping issue; no functional or maintainability impact. Filter out per selectivity criteria.</violation>
<violation number="3" location="packages/cli/src/commands/start.ts:231">
two-agent-filter: Initialization of CredentialsOverwrites duplicates packages/cli/src/commands/worker.ts:init(). This common setup logic should be centralized or abstracted if possible to avoid replication across command initializations.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Per-command init appears intentional; Start and Worker have distinct boot sequences and no shared abstraction. Duplication is minimal and not a clear bug or maintainability risk. Low impact.</violation>
<violation number="4" location="packages/cli/src/commands/start.ts:231">
single-agent-filter: Initialization of CredentialsOverwrites duplicates packages/cli/src/commands/worker.ts:init(). This common setup logic should be centralized or abstracted if possible to avoid replication across command initializations.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Accurate but low-impact: a single init call exists in both Start and Worker, likely intentional since both boot paths require credential overwrites initialized. Centralizing into BaseCommand may not be appropriate for all commands. This duplication is minimal and doesn't pose a meaningful maintainability risk.</violation>
</file>
<file name="packages/@n8n/config/src/configs/credentials.config.ts">
<violation number="1" location="packages/@n8n/config/src/configs/credentials.config.ts:20">
two-agent-filter: Misleading comment: describes an auth token but the property is a persistence flag, causing confusion.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*
• **Libraries consulted**:</violation>
<violation number="2" location="packages/@n8n/config/src/configs/credentials.config.ts:20">
single-agent-filter: Misleading comment: describes an auth token but the property is a persistence flag, causing confusion.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*
• **Libraries consulted**:</violation>
</file>
<file name="packages/cli/src/scaling/pubsub/pubsub.event-map.ts">
<violation number="1" location="packages/cli/src/scaling/pubsub/pubsub.event-map.ts:15">
two-agent-filter: Use a consistent region comment style; change to "// #region Credentials" to match other region markers in this file.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Cosmetic only; both region comment styles already exist in this file. No functional impact or actionable severity.</violation>
<violation number="2" location="packages/cli/src/scaling/pubsub/pubsub.event-map.ts:15">
single-agent-filter: Use a consistent region comment style; change to "// #region Credentials" to match other region markers in this file.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: This is a stylistic inconsistency in comment formatting. The file already mixes both styles ("#region" and "# region"), so the added lines do not uniquely introduce the inconsistency. Impact is minimal and does not affect functionality or maintainability in a meaningful way.</violation>
</file>
<file name="packages/cli/src/credentials-overwrites.ts">
<violation number="1" location="packages/cli/src/credentials-overwrites.ts:86">
two-agent-filter: The Pub/Sub synchronization mechanism introduces a race condition. It broadcasts a generic 'reload' command, causing instances to re-read from the database. This can lead to data inconsistency in a multi-instance environment if concurrent updates occur, as an instance might load a state modified by another process after the broadcast was initiated.
• **Libraries consulted**:</violation>
<violation number="2" location="packages/cli/src/credentials-overwrites.ts:86">
single-agent-filter: The Pub/Sub synchronization mechanism introduces a race condition. It broadcasts a generic 'reload' command, causing instances to re-read from the database. This can lead to data inconsistency in a multi-instance environment if concurrent updates occur, as an instance might load a state modified by another process after the broadcast was initiated.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Saving to DB then broadcasting a generic reload causes instances to fetch the latest DB state (last-write-wins). Loading a newer value after a broadcast is expected and ensures consistency with the source of truth, not a race causing incorrect state. The described scenario is not a bug with clear impact.</violation>
<violation number="3" location="packages/cli/src/credentials-overwrites.ts:149">
single-agent-filter: Avoid optional chaining after Container.get; it can mask failures to resolve the service and silently skip type regeneration. Call generateTypes directly for consistency and observability.
• **Libraries consulted**:</violation>
<violation number="4" location="packages/cli/src/credentials-overwrites.ts:149">
two-agent-filter: Avoid optional chaining after Container.get; it can mask failures to resolve the service and silently skip type regeneration. Call generateTypes directly for consistency and observability.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Container.get returns instance or throws; FrontendService is a decorated service with generateTypes(). Optional chaining is redundant, not harmful, and low-impact. Filter out.</violation>
</file>
<file name="packages/cli/src/scaling/pubsub/pubsub.types.ts">
<violation number="1" location="packages/cli/src/scaling/pubsub/pubsub.types.ts:45">
two-agent-filter: Naming inconsistency: alias uses "Overrides" but command key uses "overwrite". Consider renaming to "ReloadCredentialOverwrites" for clarity and consistency.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Stylistic naming nitpick; alias vs key wording differences are consistent with existing patterns and have no functional impact.</violation>
<violation number="2" location="packages/cli/src/scaling/pubsub/pubsub.types.ts:45">
single-agent-filter: Naming inconsistency: alias uses "Overrides" but command key uses "overwrite". Consider renaming to "ReloadCredentialOverwrites" for clarity and consistency.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Purely naming/style. The alias correctly maps to the command key; no functional impact or maintainability risk. Not worth reporting.</violation>
<violation number="3" location="packages/cli/src/scaling/pubsub/pubsub.types.ts:81">
two-agent-filter: Naming inconsistency: uses "Overrides" instead of "Overwrites". Align with command key and domain terminology (e.g., "ReloadCredentialOverwrites").
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Same low-impact naming preference; union member follows chosen alias. No technical issue or maintainability risk.</violation>
<violation number="4" location="packages/cli/src/scaling/pubsub/pubsub.types.ts:81">
single-agent-filter: Naming inconsistency: uses "Overrides" instead of "Overwrites". Align with command key and domain terminology (e.g., "ReloadCredentialOverwrites").
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Same minor naming inconsistency; the union member is consistent with the alias and does not affect functionality. Low impact.</violation>
</file>
<file name="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts">
<violation number="1" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:142">
two-agent-filter: Remove redundant await inside the map callback to simplify and avoid unnecessary async wrapping.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: commentPurpose, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Same redundant await pattern; minor style issue without functional impact.</violation>
<violation number="2" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:142">
two-agent-filter: Remove redundant await inside the map callback to simplify and avoid unnecessary async wrapping.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Technically redundant but purely stylistic with no functional impact; low importance for an integration test.</violation>
<violation number="3" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:142">
single-agent-filter: Remove redundant await inside the map callback to simplify and avoid unnecessary async wrapping.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: commentPurpose, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Same redundant await pattern as #0; minor style issue without practical impact.</violation>
<violation number="4" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:142">
single-agent-filter: Remove redundant await inside the map callback to simplify and avoid unnecessary async wrapping.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Technically correct but purely stylistic with no functional impact; not worth reporting.</violation>
<violation number="5" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:237">
two-agent-filter: This test doesn't simulate multiple instances; it triggers three reloads on a single instance. Consider constructing separate CredentialsOverwrites instances to truly validate multi-instance coordination.
• **Libraries consulted**:</violation>
<violation number="6" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:237">
single-agent-filter: This test doesn't simulate multiple instances; it triggers three reloads on a single instance. Consider constructing separate CredentialsOverwrites instances to truly validate multi-instance coordination.
• **Libraries consulted**:</violation>
<violation number="7" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:329">
two-agent-filter: Relax the hard 2s threshold to reduce CI flakiness for DB operations triggered by PubSub reloads.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: commentPurpose.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Same as #3: speculative performance threshold change without concrete instability evidence; low importance.</violation>
<violation number="8" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:329">
two-agent-filter: Relax the hard 2s threshold to reduce CI flakiness for DB operations.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: commentPurpose.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Suggestion to relax a 2s threshold is speculative without evidence of CI flakiness; low impact and subjective.</violation>
<violation number="9" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:329">
single-agent-filter: Relax the hard 2s threshold to reduce CI flakiness for DB operations triggered by PubSub reloads.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: commentPurpose.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Same as #3; performance threshold adjustment is speculative and low impact without evidence.</violation>
<violation number="10" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:329">
single-agent-filter: Relax the hard 2s threshold to reduce CI flakiness for DB operations.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: commentPurpose.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Time-based threshold change is speculative; no evidence of CI flakiness. 2s for three sequential DB operations is reasonable and changing it provides minimal value.</violation>
</file>
<file name="packages/cli/src/server.ts">
<violation number="1" location="packages/cli/src/server.ts:300">
two-agent-filter: Simplify and align style by coercing to boolean for readability and consistency with the check above.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: commentPurpose, typescriptUnderstanding, functionalImpact, objectivity.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Stylistic-only: !== null already produces a boolean and is clear; changing to !! has no functional benefit.</violation>
<violation number="2" location="packages/cli/src/server.ts:300">
single-agent-filter: Simplify and align style by coercing to boolean for readability and consistency with the check above.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: commentPurpose, typescriptUnderstanding, functionalImpact, objectivity.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: getOverwriteEndpointMiddleware() returns either a middleware function or null. Using !== null is functionally correct and equivalent to the prior truthy check in this context. Changing to boolean coercion would be purely stylistic with no functional impact; not worth reporting.</violation>
<violation number="3" location="packages/cli/src/server.ts:318">
two-agent-filter: Duplicate frontend types generation: setData defaults to reloading frontend, then generateTypes() is called again. Pass reloadFrontend=false to avoid redundant work.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
• **Libraries consulted**:</violation>
<violation number="4" location="packages/cli/src/server.ts:318">
single-agent-filter: Duplicate frontend types generation: setData defaults to reloading frontend, then generateTypes() is called again. Pass reloadFrontend=false to avoid redundant work.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
• **Libraries consulted**:</violation>
</file>
<file name="packages/cli/src/scaling/worker-server.ts">
<violation number="1" location="packages/cli/src/scaling/worker-server.ts:159">
two-agent-filter: Potential race: awaiting setData() before setting `overwritesLoaded` allows two concurrent requests to pass the guard and persist overwrites twice. Consider setting a loading flag before the await or using an atomic guard to prevent concurrent execution.
• **Libraries consulted**:</violation>
<violation number="2" location="packages/cli/src/scaling/worker-server.ts:159">
single-agent-filter: Potential race: awaiting setData() before setting `overwritesLoaded` allows two concurrent requests to pass the guard and persist overwrites twice. Consider setting a loading flag before the await or using an atomic guard to prevent concurrent execution.
• **Libraries consulted**:</violation>
</file>
<file name="packages/cli/src/__tests__/credentials-overwrites.test.ts">
<violation number="1" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:445">
two-agent-filter: Use a valid async assertion; 'resolves.not.toThrow' is inappropriate for promises.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of violation #0; same invalid pattern ‘resolves.not.toThrow’.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher, Jestjs_io-docs-getting-started</violation>
<violation number="2" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:445">
two-agent-filter: Invalid Jest assertion on promise: replace 'resolves.not.toThrow' with a valid resolution assertion.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of violation #0; same invalid ‘resolves.not.toThrow’ usage in a different test context.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher, Jestjs_io-docs-getting-started</violation>
<violation number="3" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:445">
two-agent-filter: Invalid Jest assertion: 'resolves.not.toThrow' is not valid for promises; assert resolution instead (e.g., resolves.toBeUndefined).
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher, Jestjs_io-docs-getting-started</violation>
<violation number="4" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:445">
single-agent-filter: Use a valid async assertion; 'resolves.not.toThrow' is inappropriate for promises.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of #0: same invalid matcher chain repeated. Covered by the kept violation.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started</violation>
<violation number="5" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:445">
single-agent-filter: Invalid Jest assertion on promise: replace 'resolves.not.toThrow' with a valid resolution assertion.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of #0: same invalid resolves.not.toThrow usage in another test. Keep only one instance to reduce noise.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started</violation>
<violation number="6" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:445">
single-agent-filter: Invalid Jest assertion: 'resolves.not.toThrow' is not valid for promises; assert resolution instead (e.g., resolves.toBeUndefined).
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started</violation>
<violation number="7" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:611">
two-agent-filter: Replace invalid 'resolves.not.toThrow' with a proper resolution assertion.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of violation #0; same invalid ‘resolves.not.toThrow’ usage.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher, Jestjs_io-docs-getting-started</violation>
<violation number="8" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:611">
two-agent-filter: Fix invalid async assertion by asserting the promise resolves (e.g., resolves.toBeUndefined).
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of violation #0; same invalid ‘resolves.not.toThrow’ matcher chain.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher, Jestjs_io-docs-getting-started</violation>
<violation number="9" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:611">
two-agent-filter: Invalid Jest matcher chain for async code; replace 'resolves.not.toThrow' with a resolution assertion.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of violation #0; same invalid ‘resolves.not.toThrow’ matcher chain.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher, Jestjs_io-docs-getting-started</violation>
<violation number="10" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:611">
single-agent-filter: Replace invalid 'resolves.not.toThrow' with a proper resolution assertion.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of the same invalid matcher chain; redundant with #0.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started</violation>
<violation number="11" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:611">
single-agent-filter: Fix invalid async assertion by asserting the promise resolves (e.g., resolves.toBeUndefined).
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of the invalid resolves.not.toThrow usage; already covered by #0.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started</violation>
<violation number="12" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:611">
single-agent-filter: Invalid Jest matcher chain for async code; replace 'resolves.not.toThrow' with a resolution assertion.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate pattern of #0 in a different describe block. Issue is the same invalid use of resolves.not.toThrow; already covered.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started</violation>
<violation number="13" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:755">
two-agent-filter: Comment contradicts the test assertion; update the comment to reflect that an error is expected.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher, Jestjs_io-docs-getting-started</violation>
<violation number="14" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:755">
single-agent-filter: Comment contradicts the test assertion; update the comment to reflect that an error is expected.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| Container.get(WaitTracker).init(); | ||
| this.logger.debug('Wait tracker init complete'); | ||
| await Container.get(CredentialsOverwrites).init(); |
There was a problem hiding this comment.
two-agent-filter: Initialization of CredentialsOverwrites duplicates packages/cli/src/commands/worker.ts:init(). This common setup logic should be centralized or abstracted if possible to avoid replication across command initializations.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Per-command init appears intentional; Start and Worker have distinct boot sequences and no shared abstraction. Duplication is minimal and not a clear bug or maintainability risk. Low impact.
Prompt for AI agents
Address the following comment on packages/cli/src/commands/start.ts at line 231:
<comment>two-agent-filter: Initialization of CredentialsOverwrites duplicates packages/cli/src/commands/worker.ts:init(). This common setup logic should be centralized or abstracted if possible to avoid replication across command initializations.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Per-command init appears intentional; Start and Worker have distinct boot sequences and no shared abstraction. Duplication is minimal and not a clear bug or maintainability risk. Low impact.</comment>
<file context>
@@ -227,6 +228,8 @@ export class Start extends BaseCommand<z.infer<typeof flagsSchema>> {
Container.get(WaitTracker).init();
this.logger.debug('Wait tracker init complete');
+ await Container.get(CredentialsOverwrites).init();
+ this.logger.debug('Credentials overwrites init complete');
await this.initBinaryDataService();
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Unmapped Agent in getAgentNameFromViolationSource (undefined)
|
|
||
| Container.get(WaitTracker).init(); | ||
| this.logger.debug('Wait tracker init complete'); | ||
| await Container.get(CredentialsOverwrites).init(); |
There was a problem hiding this comment.
single-agent-filter: Initialization of CredentialsOverwrites duplicates packages/cli/src/commands/worker.ts:init(). This common setup logic should be centralized or abstracted if possible to avoid replication across command initializations.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Accurate but low-impact: a single init call exists in both Start and Worker, likely intentional since both boot paths require credential overwrites initialized. Centralizing into BaseCommand may not be appropriate for all commands. This duplication is minimal and doesn't pose a meaningful maintainability risk.
Prompt for AI agents
Address the following comment on packages/cli/src/commands/start.ts at line 231:
<comment>single-agent-filter: Initialization of CredentialsOverwrites duplicates packages/cli/src/commands/worker.ts:init(). This common setup logic should be centralized or abstracted if possible to avoid replication across command initializations.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Accurate but low-impact: a single init call exists in both Start and Worker, likely intentional since both boot paths require credential overwrites initialized. Centralizing into BaseCommand may not be appropriate for all commands. This duplication is minimal and doesn't pose a meaningful maintainability risk.</comment>
<file context>
@@ -227,6 +228,8 @@ export class Start extends BaseCommand<z.infer<typeof flagsSchema>> {
Container.get(WaitTracker).init();
this.logger.debug('Wait tracker init complete');
+ await Container.get(CredentialsOverwrites).init();
+ this.logger.debug('Credentials overwrites init complete');
await this.initBinaryDataService();
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Unmapped Agent in getAgentNameFromViolationSource (undefined)
| @Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN') | ||
| endpointAuthToken: string = ''; | ||
|
|
||
| /** Authentication token for the credentials overwrite endpoint. */ |
There was a problem hiding this comment.
two-agent-filter: Misleading comment: describes an auth token but the property is a persistence flag, causing confusion.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.
• Libraries consulted:
Prompt for AI agents
Address the following comment on packages/@n8n/config/src/configs/credentials.config.ts at line 20:
<comment>two-agent-filter: Misleading comment: describes an auth token but the property is a persistence flag, causing confusion.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*
• **Libraries consulted**: </comment>
<file context>
@@ -16,6 +16,10 @@ class CredentialsOverwrite {
@Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN')
endpointAuthToken: string = '';
+
+ /** Authentication token for the credentials overwrite endpoint. */
+ @Env('CREDENTIALS_OVERWRITE_PERSISTENCE')
+ persistence: boolean = false;
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: General AI Review Agent
Suggested change
| /** Authentication token for the credentials overwrite endpoint. */ | |
| /** Enable persistence of credential overwrites. */ |
| @Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN') | ||
| endpointAuthToken: string = ''; | ||
|
|
||
| /** Authentication token for the credentials overwrite endpoint. */ |
There was a problem hiding this comment.
single-agent-filter: Misleading comment: describes an auth token but the property is a persistence flag, causing confusion.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.
• Libraries consulted:
Prompt for AI agents
Address the following comment on packages/@n8n/config/src/configs/credentials.config.ts at line 20:
<comment>single-agent-filter: Misleading comment: describes an auth token but the property is a persistence flag, causing confusion.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*
• **Libraries consulted**: </comment>
<file context>
@@ -16,6 +16,10 @@ class CredentialsOverwrite {
@Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN')
endpointAuthToken: string = '';
+
+ /** Authentication token for the credentials overwrite endpoint. */
+ @Env('CREDENTIALS_OVERWRITE_PERSISTENCE')
+ persistence: boolean = false;
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: General AI Review Agent
Suggested change
| /** Authentication token for the credentials overwrite endpoint. */ | |
| /** Enable persistence of credential overwrites. */ |
| import { WorkflowRunner } from '@/workflow-runner'; | ||
|
|
||
| import { BaseCommand } from './base-command'; | ||
| import { CredentialsOverwrites } from '@/credentials-overwrites'; |
There was a problem hiding this comment.
single-agent-filter: Import from '@/credentials-overwrites' is placed after a relative import, breaking the existing alias-vs-relative import grouping. Consider moving it to the block with other '@/…' imports for consistency.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact, objectivity.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Technically accurate (alias import placed after a relative import), but purely stylistic with no functional impact. This is a low-severity consistency nit that should be handled by linting/formatting rules if desired, not a high-impact issue worth flagging.
Prompt for AI agents
Address the following comment on packages/cli/src/commands/start.ts at line 36:
<comment>single-agent-filter: Import from '@/credentials-overwrites' is placed after a relative import, breaking the existing alias-vs-relative import grouping. Consider moving it to the block with other '@/…' imports for consistency.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact, objectivity.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Technically accurate (alias import placed after a relative import), but purely stylistic with no functional impact. This is a low-severity consistency nit that should be handled by linting/formatting rules if desired, not a high-impact issue worth flagging.</comment>
<file context>
@@ -33,6 +33,7 @@ import { WaitTracker } from '@/wait-tracker';
import { WorkflowRunner } from '@/workflow-runner';
import { BaseCommand } from './base-command';
+import { CredentialsOverwrites } from '@/credentials-overwrites';
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
</file context>
[internal] Confidence score: 6/10
[internal] Posted by: General AI Review Agent
| }); | ||
|
|
||
| // Should not throw but log error | ||
| await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow(); |
There was a problem hiding this comment.
single-agent-filter: Use a valid async assertion; 'resolves.not.toThrow' is inappropriate for promises.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Duplicate of #0: same invalid matcher chain repeated. Covered by the kept violation.
• Libraries consulted: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started
Prompt for AI agents
Address the following comment on packages/cli/src/__tests__/credentials-overwrites.test.ts at line 445:
<comment>single-agent-filter: Use a valid async assertion; 'resolves.not.toThrow' is inappropriate for promises.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of #0: same invalid matcher chain repeated. Covered by the kept violation.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started</comment>
<file context>
@@ -234,4 +254,963 @@ describe('CredentialsOverwrites', () => {
+ });
+
+ // Should not throw but log error
+ await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow();
+
+ expect(settingsRepository.findByKey).toHaveBeenCalledWith('credentialsOverwrite');
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
Suggested change
| await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow(); | |
| await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.toBeUndefined(); |
| }); | ||
|
|
||
| // Should not throw but log error | ||
| await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow(); |
There was a problem hiding this comment.
single-agent-filter: Invalid Jest assertion on promise: replace 'resolves.not.toThrow' with a valid resolution assertion.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Duplicate of #0: same invalid resolves.not.toThrow usage in another test. Keep only one instance to reduce noise.
• Libraries consulted: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started
Prompt for AI agents
Address the following comment on packages/cli/src/__tests__/credentials-overwrites.test.ts at line 445:
<comment>single-agent-filter: Invalid Jest assertion on promise: replace 'resolves.not.toThrow' with a valid resolution assertion.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of #0: same invalid resolves.not.toThrow usage in another test. Keep only one instance to reduce noise.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started</comment>
<file context>
@@ -234,4 +254,963 @@ describe('CredentialsOverwrites', () => {
+ });
+
+ // Should not throw but log error
+ await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow();
+
+ expect(settingsRepository.findByKey).toHaveBeenCalledWith('credentialsOverwrite');
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
Suggested change
| await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow(); | |
| await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.toBeUndefined(); |
| }); | ||
|
|
||
| // Should not throw but log error | ||
| await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow(); |
There was a problem hiding this comment.
single-agent-filter: Invalid Jest assertion: 'resolves.not.toThrow' is not valid for promises; assert resolution instead (e.g., resolves.toBeUndefined).
• Libraries consulted: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started
Prompt for AI agents
Address the following comment on packages/cli/src/__tests__/credentials-overwrites.test.ts at line 445:
<comment>single-agent-filter: Invalid Jest assertion: 'resolves.not.toThrow' is not valid for promises; assert resolution instead (e.g., resolves.toBeUndefined).
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher usage, Jestjs_io-docs-getting-started</comment>
<file context>
@@ -234,4 +254,963 @@ describe('CredentialsOverwrites', () => {
+ });
+
+ // Should not throw but log error
+ await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow();
+
+ expect(settingsRepository.findByKey).toHaveBeenCalledWith('credentialsOverwrite');
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
Suggested change
| await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow(); | |
| await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.toBeUndefined(); |
| // FrontendService has CredentialOverwrites injected via the constructor | ||
| // to break the circular dependency we need to use the container to get the instance | ||
| const { FrontendService } = await import('./services/frontend.service'); | ||
| await Container.get(FrontendService)?.generateTypes(); |
There was a problem hiding this comment.
single-agent-filter: Avoid optional chaining after Container.get; it can mask failures to resolve the service and silently skip type regeneration. Call generateTypes directly for consistency and observability.
• Libraries consulted:
Prompt for AI agents
Address the following comment on packages/cli/src/credentials-overwrites.ts at line 149:
<comment>single-agent-filter: Avoid optional chaining after Container.get; it can mask failures to resolve the service and silently skip type regeneration. Call generateTypes directly for consistency and observability.
• **Libraries consulted**: </comment>
<file context>
@@ -58,6 +132,21 @@ export class CredentialsOverwrites {
+ // FrontendService has CredentialOverwrites injected via the constructor
+ // to break the circular dependency we need to use the container to get the instance
+ const { FrontendService } = await import('./services/frontend.service');
+ await Container.get(FrontendService)?.generateTypes();
}
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
| // FrontendService has CredentialOverwrites injected via the constructor | ||
| // to break the circular dependency we need to use the container to get the instance | ||
| const { FrontendService } = await import('./services/frontend.service'); | ||
| await Container.get(FrontendService)?.generateTypes(); |
There was a problem hiding this comment.
two-agent-filter: Avoid optional chaining after Container.get; it can mask failures to resolve the service and silently skip type regeneration. Call generateTypes directly for consistency and observability.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Container.get returns instance or throws; FrontendService is a decorated service with generateTypes(). Optional chaining is redundant, not harmful, and low-impact. Filter out.
Prompt for AI agents
Address the following comment on packages/cli/src/credentials-overwrites.ts at line 149:
<comment>two-agent-filter: Avoid optional chaining after Container.get; it can mask failures to resolve the service and silently skip type regeneration. Call generateTypes directly for consistency and observability.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Container.get returns instance or throws; FrontendService is a decorated service with generateTypes(). Optional chaining is redundant, not harmful, and low-impact. Filter out.</comment>
<file context>
@@ -58,6 +132,21 @@ export class CredentialsOverwrites {
+ // FrontendService has CredentialOverwrites injected via the constructor
+ // to break the circular dependency we need to use the container to get the instance
+ const { FrontendService } = await import('./services/frontend.service');
+ await Container.get(FrontendService)?.generateTypes();
}
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
There was a problem hiding this comment.
19 issues found across 12 files
Prompt for AI agents (all 19 issues)
Understand the root cause of the following 19 issues and fix them.
<file name="packages/@n8n/config/src/configs/credentials.config.ts">
<violation number="1" location="packages/@n8n/config/src/configs/credentials.config.ts:20">
Incorrect JSDoc: describes an authentication token, but the field is a boolean persistence flag. Update the comment to accurately reflect the purpose.</violation>
<violation number="2" location="packages/@n8n/config/src/configs/credentials.config.ts:22">
Consider renaming the boolean to match existing conventions (e.g., persistenceEnabled) for clarity and consistency.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact, objectivity.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: This is a stylistic naming suggestion with no functional impact. Conventions in the codebase are mixed and the current name matches the env var, which aids discoverability. Low importance.</violation>
</file>
<file name="packages/cli/src/commands/start.ts">
<violation number="1" location="packages/cli/src/commands/start.ts:36">
Alias import is placed after a relative import, breaking import-order grouping. Move the `@/...` import to be with other internal imports above relative ones for consistency.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Technically accurate but purely stylistic: an alias import (‘@/...’) appears after a relative import, whereas other alias imports are grouped above. There is no functional impact, and no repository-enforced rule found for import ordering (no import/order config present). Under medium sensitivity, this is a nit and not worth reporting.</violation>
</file>
<file name="packages/cli/src/server.ts">
<violation number="1" location="packages/cli/src/server.ts:300">
Use boolean coercion for clarity and robustness: derive authenticationEnforced via truthiness rather than explicit null comparison.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact, objectivity.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Stylistic preference: explicit null comparison is clear and correct; switching to boolean coercion has no functional impact.</violation>
<violation number="2" location="packages/cli/src/server.ts:318">
setData(..., true) defaults to reloadFrontend = true, which already regenerates UI types; combined with the subsequent generateTypes(), this duplicates work. Pass false for reloadFrontend to avoid redundant regeneration.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*</violation>
</file>
<file name="packages/cli/src/__tests__/credentials-overwrites.test.ts">
<violation number="1" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:445">
Invalid Jest assertion: .resolves.not.toThrow() is not valid on a Promise; use .resolves.toBeUndefined() or simply await.</violation>
<violation number="2" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:611">
Invalid Jest assertion: .resolves.not.toThrow() is not valid on a Promise; use .resolves.toBeUndefined() or simply await.</violation>
<violation number="3" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:755">
Misleading comment contradicts the test assertion; update the comment to reflect that an error is expected.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Contradictory comment is a minor documentation issue with low impact; filter out per selectivity guidelines.
• **Libraries consulted**: Jest expect toThrow resolves, Jestjs_io-docs-getting-started</violation>
</file>
<file name="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts">
<violation number="1" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:54">
Test name is misleading; it claims Frontend reload, but the test explicitly disables it. Rename for clarity.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Accurate but minor naming inconsistency; no functional impact. Medium sensitivity: filter low-impact stylistic issues.</violation>
<violation number="2" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:142">
Remove redundant async/await in map callback to simplify and avoid unnecessary microtasks.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Redundant async/await in map is a minor style issue with negligible impact; code is correct. Filter.</violation>
<violation number="3" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:209">
Test name overstates scope; it doesn't verify PubSub or multi-instance behavior. Rename for accuracy.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Overstated title without functional impact; naming-only concern. Filter.</violation>
<violation number="4" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:224">
Test name is misleading; it uses a single instance with repeated calls. Rename to reflect actual behavior.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Misleading “multiple instances” wording but single-instance behavior; naming-only. Filter.</violation>
<violation number="5" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:250">
Test title does not match behavior; it avoids DB interactions. Rename to accurately describe the memory-only path.</violation>
<violation number="6" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:280">
Test name is misleading; first step does not exercise DB failure. Rename for clarity.</violation>
<violation number="7" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:329">
Relax strict time-based assertion to avoid flaky failures across environments.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Same as #7; speculative flakiness concern. Filter.</violation>
</file>
<file name="packages/cli/src/commands/worker.ts">
<violation number="1" location="packages/cli/src/commands/worker.ts:92">
Concurrency may be set to 0 or negative, leading to invalid queue.process concurrency and worker malfunction.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Bull allows concurrency=0 per official docs; negative values concern is speculative; cited location unrelated. Low confidence — filter.
• **Libraries consulted**: Bull queue.process concurrency (OptimalBits/bull), Bull</violation>
</file>
<file name="packages/cli/src/scaling/worker-server.ts">
<violation number="1" location="packages/cli/src/scaling/worker-server.ts:124">
Redundant await in async route wrapper; return the promise from `handleOverwrites` directly for brevity.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact, objectivity.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Awaiting inside async Express 5 handler vs returning promise is behaviorally equivalent; style-only, no impact.</violation>
<violation number="2" location="packages/cli/src/scaling/worker-server.ts:159">
Redundant boolean argument `true`; `setData` defaults already set `storeInDb` and `reloadFrontend` to true. Removing it improves readability.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Explicit true matches defaults; purely stylistic, no functional/maintainability impact.</violation>
<violation number="3" location="packages/cli/src/scaling/worker-server.ts:159">
Awaiting the DB write before setting `overwritesLoaded` introduces a race window allowing concurrent overwrite submissions to bypass the guard. Consider setting a guard flag before the await or adding synchronization to prevent concurrent execution.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| /** Authentication token for the credentials overwrite endpoint. */ | ||
| @Env('CREDENTIALS_OVERWRITE_PERSISTENCE') | ||
| persistence: boolean = false; |
There was a problem hiding this comment.
Consider renaming the boolean to match existing conventions (e.g., persistenceEnabled) for clarity and consistency.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact, objectivity.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: This is a stylistic naming suggestion with no functional impact. Conventions in the codebase are mixed and the current name matches the env var, which aids discoverability. Low importance.
Prompt for AI agents
Address the following comment on packages/@n8n/config/src/configs/credentials.config.ts at line 22:
<comment>Consider renaming the boolean to match existing conventions (e.g., persistenceEnabled) for clarity and consistency.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact, objectivity.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: This is a stylistic naming suggestion with no functional impact. Conventions in the codebase are mixed and the current name matches the env var, which aids discoverability. Low importance.</comment>
<file context>
@@ -16,6 +16,10 @@ class CredentialsOverwrite {
+
+ /** Authentication token for the credentials overwrite endpoint. */
+ @Env('CREDENTIALS_OVERWRITE_PERSISTENCE')
+ persistence: boolean = false;
}
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: General AI Review Agent
| @Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN') | ||
| endpointAuthToken: string = ''; | ||
|
|
||
| /** Authentication token for the credentials overwrite endpoint. */ |
There was a problem hiding this comment.
Incorrect JSDoc: describes an authentication token, but the field is a boolean persistence flag. Update the comment to accurately reflect the purpose.
Prompt for AI agents
Address the following comment on packages/@n8n/config/src/configs/credentials.config.ts at line 20:
<comment>Incorrect JSDoc: describes an authentication token, but the field is a boolean persistence flag. Update the comment to accurately reflect the purpose.</comment>
<file context>
@@ -16,6 +16,10 @@ class CredentialsOverwrite {
@Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN')
endpointAuthToken: string = '';
+
+ /** Authentication token for the credentials overwrite endpoint. */
+ @Env('CREDENTIALS_OVERWRITE_PERSISTENCE')
+ persistence: boolean = false;
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: General AI Review Agent
Suggested change
| /** Authentication token for the credentials overwrite endpoint. */ | |
| /** Enable persistence of credential overwrites. */ |
| import { WorkflowRunner } from '@/workflow-runner'; | ||
|
|
||
| import { BaseCommand } from './base-command'; | ||
| import { CredentialsOverwrites } from '@/credentials-overwrites'; |
There was a problem hiding this comment.
Alias import is placed after a relative import, breaking import-order grouping. Move the @/... import to be with other internal imports above relative ones for consistency.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Technically accurate but purely stylistic: an alias import (‘@/...’) appears after a relative import, whereas other alias imports are grouped above. There is no functional impact, and no repository-enforced rule found for import ordering (no import/order config present). Under medium sensitivity, this is a nit and not worth reporting.
Prompt for AI agents
Address the following comment on packages/cli/src/commands/start.ts at line 36:
<comment>Alias import is placed after a relative import, breaking import-order grouping. Move the `@/...` import to be with other internal imports above relative ones for consistency.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Technically accurate but purely stylistic: an alias import (‘@/...’) appears after a relative import, whereas other alias imports are grouped above. There is no functional impact, and no repository-enforced rule found for import ordering (no import/order config present). Under medium sensitivity, this is a nit and not worth reporting.</comment>
<file context>
@@ -33,6 +33,7 @@ import { WaitTracker } from '@/wait-tracker';
import { WorkflowRunner } from '@/workflow-runner';
import { BaseCommand } from './base-command';
+import { CredentialsOverwrites } from '@/credentials-overwrites';
// eslint-disable-next-line @typescript-eslint/no-unsafe-assignment
</file context>
[internal] Confidence score: 6/10
[internal] Posted by: General AI Review Agent
| this.app.use(`/${this.endpointPresetCredentials}`, overwriteEndpointMiddleware); | ||
| } | ||
|
|
||
| const authenticationEnforced = overwriteEndpointMiddleware !== null; |
There was a problem hiding this comment.
Use boolean coercion for clarity and robustness: derive authenticationEnforced via truthiness rather than explicit null comparison.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact, objectivity.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Stylistic preference: explicit null comparison is clear and correct; switching to boolean coercion has no functional impact.
Prompt for AI agents
Address the following comment on packages/cli/src/server.ts at line 300:
<comment>Use boolean coercion for clarity and robustness: derive authenticationEnforced via truthiness rather than explicit null comparison.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact, objectivity.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Stylistic preference: explicit null comparison is clear and correct; switching to boolean coercion has no functional impact.</comment>
<file context>
@@ -297,10 +297,12 @@ export class Server extends AbstractServer {
this.app.use(`/${this.endpointPresetCredentials}`, overwriteEndpointMiddleware);
}
+ const authenticationEnforced = overwriteEndpointMiddleware !== null;
this.app.post(
`/${this.endpointPresetCredentials}`,
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: General AI Review Agent
| } | ||
|
|
||
| Container.get(CredentialsOverwrites).setData(body); | ||
| await Container.get(CredentialsOverwrites).setData(body, true); |
There was a problem hiding this comment.
setData(..., true) defaults to reloadFrontend = true, which already regenerates UI types; combined with the subsequent generateTypes(), this duplicates work. Pass false for reloadFrontend to avoid redundant regeneration.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.
Prompt for AI agents
Address the following comment on packages/cli/src/server.ts at line 318:
<comment>setData(..., true) defaults to reloadFrontend = true, which already regenerates UI types; combined with the subsequent generateTypes(), this duplicates work. Pass false for reloadFrontend to avoid redundant regeneration.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*</comment>
<file context>
@@ -313,7 +315,7 @@ export class Server extends AbstractServer {
}
- Container.get(CredentialsOverwrites).setData(body);
+ await Container.get(CredentialsOverwrites).setData(body, true);
await frontendService?.generateTypes();
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
|
|
||
| describe('Integration Test Coverage', () => { | ||
| describe('Complete Credential Update Flow', () => { | ||
| it('should execute full API → Database → PubSub → Frontend chain', async () => { |
There was a problem hiding this comment.
Test name is misleading; it claims Frontend reload, but the test explicitly disables it. Rename for clarity.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Accurate but minor naming inconsistency; no functional impact. Medium sensitivity: filter low-impact stylistic issues.
Prompt for AI agents
Address the following comment on packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts at line 54:
<comment>Test name is misleading; it claims Frontend reload, but the test explicitly disables it. Rename for clarity.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Accurate but minor naming inconsistency; no functional impact. Medium sensitivity: filter low-impact stylistic issues.</comment>
<file context>
@@ -0,0 +1,503 @@
+
+ describe('Integration Test Coverage', () => {
+ describe('Complete Credential Update Flow', () => {
+ it('should execute full API → Database → PubSub → Frontend chain', async () => {
+ const testOverwriteData: ICredentialsOverwrite = {
+ test: { username: 'integrationUser', password: 'integrationPass' },
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
|
|
||
| await this.initLicense(); | ||
| this.logger.debug('License init complete'); | ||
| await Container.get(CredentialsOverwrites).init(); |
There was a problem hiding this comment.
Concurrency may be set to 0 or negative, leading to invalid queue.process concurrency and worker malfunction.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Bull allows concurrency=0 per official docs; negative values concern is speculative; cited location unrelated. Low confidence — filter.
• Libraries consulted: Bull queue.process concurrency (OptimalBits/bull), Bull
Prompt for AI agents
Address the following comment on packages/cli/src/commands/worker.ts at line 92:
<comment>Concurrency may be set to 0 or negative, leading to invalid queue.process concurrency and worker malfunction.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Bull allows concurrency=0 per official docs; negative values concern is speculative; cited location unrelated. Low confidence — filter.
• **Libraries consulted**: Bull queue.process concurrency (OptimalBits/bull), Bull</comment>
<file context>
@@ -88,6 +89,8 @@ export class Worker extends BaseCommand<z.infer<typeof flagsSchema>> {
await this.initLicense();
this.logger.debug('License init complete');
+ await Container.get(CredentialsOverwrites).init();
+ this.logger.debug('Credentials overwrites init complete');
await this.initBinaryDataService();
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: Functional Bugs Agent
| this.handleOverwrites(req, res), | ||
| ); | ||
| this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, async (req, res) => { | ||
| await this.handleOverwrites(req, res); |
There was a problem hiding this comment.
Redundant await in async route wrapper; return the promise from handleOverwrites directly for brevity.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact, objectivity.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Awaiting inside async Express 5 handler vs returning promise is behaviorally equivalent; style-only, no impact.
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/worker-server.ts at line 124:
<comment>Redundant await in async route wrapper; return the promise from `handleOverwrites` directly for brevity.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact, objectivity.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Awaiting inside async Express 5 handler vs returning promise is behaviorally equivalent; style-only, no impact.</comment>
<file context>
@@ -120,9 +120,9 @@ export class WorkerServer {
- this.handleOverwrites(req, res),
- );
+ this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, async (req, res) => {
+ await this.handleOverwrites(req, res);
+ });
}
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
| } | ||
|
|
||
| this.credentialsOverwrites.setData(req.body); | ||
| await this.credentialsOverwrites.setData(req.body, true); |
There was a problem hiding this comment.
Redundant boolean argument true; setData defaults already set storeInDb and reloadFrontend to true. Removing it improves readability.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Explicit true matches defaults; purely stylistic, no functional/maintainability impact.
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/worker-server.ts at line 159:
<comment>Redundant boolean argument `true`; `setData` defaults already set `storeInDb` and `reloadFrontend` to true. Removing it improves readability.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding, functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Explicit true matches defaults; purely stylistic, no functional/maintainability impact.</comment>
<file context>
@@ -156,7 +156,7 @@ export class WorkerServer {
}
- this.credentialsOverwrites.setData(req.body);
+ await this.credentialsOverwrites.setData(req.body, true);
this.overwritesLoaded = true;
</file context>
[internal] Confidence score: 10/10
[internal] Posted by: General AI Review Agent
| } | ||
|
|
||
| this.credentialsOverwrites.setData(req.body); | ||
| await this.credentialsOverwrites.setData(req.body, true); |
There was a problem hiding this comment.
Awaiting the DB write before setting overwritesLoaded introduces a race window allowing concurrent overwrite submissions to bypass the guard. Consider setting a guard flag before the await or adding synchronization to prevent concurrent execution.
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/worker-server.ts at line 159:
<comment>Awaiting the DB write before setting `overwritesLoaded` introduces a race window allowing concurrent overwrite submissions to bypass the guard. Consider setting a guard flag before the await or adding synchronization to prevent concurrent execution.</comment>
<file context>
@@ -156,7 +156,7 @@ export class WorkerServer {
}
- this.credentialsOverwrites.setData(req.body);
+ await this.credentialsOverwrites.setData(req.body, true);
this.overwritesLoaded = true;
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
There was a problem hiding this comment.
9 issues found across 12 files
Prompt for AI agents (all 9 issues)
Understand the root cause of the following 9 issues and fix them.
<file name="packages/cli/src/scaling/pubsub/pubsub.event-map.ts">
<violation number="1" location="packages/cli/src/scaling/pubsub/pubsub.event-map.ts:15">
Opening region comment uses `# region` (with a space) but closes with `#endregion`. Use `#region Credentials` for consistency and proper editor folding.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Technically accurate but low-impact stylistic inconsistency in region comment spacing; no functional effect and similar pattern already exists in the file. Filter under medium sensitivity.</violation>
</file>
<file name="packages/cli/src/server.ts">
<violation number="1" location="packages/cli/src/server.ts:318">
Avoid duplicate UI types regeneration. setData(body, true) already reloads the frontend by default, and calling frontendService.generateTypes() afterward redundantly repeats the operation. Pass reloadFrontend: false to setData here to prevent duplication.</violation>
</file>
<file name="packages/cli/src/scaling/pubsub/pubsub.types.ts">
<violation number="1" location="packages/cli/src/scaling/pubsub/pubsub.types.ts:45">
Alias name uses "Overrides" but the event key and feature terminology use "overwrite". Rename for consistency to avoid confusion.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Accurate that alias uses “Overrides” while the event key/feature use “overwrite”, confirmed by code. However, alias correctly maps to the event key, yielding no functional or type impact. This is a minor naming consistency issue and not worth reporting under medium sensitivity.</violation>
</file>
<file name="packages/cli/src/scaling/worker-server.ts">
<violation number="1" location="packages/cli/src/scaling/worker-server.ts:123">
Async route lacks error handling; thrown/rejected errors from setData cause unhandled promise rejection and no error response in Express 4
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Express 5.1.0 is used; async handler errors are automatically forwarded to next(err) and produce a 500 response by default. The claim assumes Express 4 behavior, so it’s not applicable. No unhandled rejection or missing error response.
• **Libraries consulted**: Express 5 async route error handling, Expressjs-en-5x</violation>
<violation number="2" location="packages/cli/src/scaling/worker-server.ts:159">
Awaiting `setData` before setting the `overwritesLoaded` guard introduces a race: concurrent requests can bypass the guard and trigger duplicate writes and reloads. Consider setting the guard before the await or adding an internal mutex.</violation>
</file>
<file name="packages/cli/src/__tests__/credentials-overwrites.test.ts">
<violation number="1" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:445">
Invalid Jest assertion: toThrow expects a function, but it's chained after .resolves on a promise. Use .resolves.toBeUndefined() (or assert a specific resolved value) to verify no rejection occurs.</violation>
<violation number="2" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:611">
Invalid Jest assertion: toThrow expects a function, not a resolved value. Replace with .resolves.toBeUndefined() to assert the async call does not reject.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of #0 (same invalid .resolves.not.toThrow() pattern in PubSub tests). Covered by keeping #0.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher, Jestjs_io-docs-getting-started, Jest doMock module mocking caching resetModules isolateModules</violation>
<violation number="3" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:722">
Test description/comment conflicts with assertion: it claims errors are handled gracefully (no throw) but expects a rejection. Align the description or the assertion to avoid misleading tests.</violation>
<violation number="4" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:744">
jest.doMock is applied after the module was already imported in beforeEach, so the mock likely won't take effect due to module caching. This makes the test unreliable; reset/isolate modules before mocking or avoid the earlier import.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
|
|
||
| // #endregion | ||
|
|
||
| // # region Credentials |
There was a problem hiding this comment.
Opening region comment uses # region (with a space) but closes with #endregion. Use #region Credentials for consistency and proper editor folding.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Technically accurate but low-impact stylistic inconsistency in region comment spacing; no functional effect and similar pattern already exists in the file. Filter under medium sensitivity.
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/pubsub/pubsub.event-map.ts at line 15:
<comment>Opening region comment uses `# region` (with a space) but closes with `#endregion`. Use `#region Credentials` for consistency and proper editor folding.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Technically accurate but low-impact stylistic inconsistency in region comment spacing; no functional effect and similar pattern already exists in the file. Filter under medium sensitivity.</comment>
<file context>
@@ -12,6 +12,10 @@ export type PubSubCommandMap = {
// #endregion
+ // # region Credentials
+ 'reload-overwrite-credentials': never;
+ // #endregion
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
Suggested change
| // # region Credentials | |
| // #region Credentials |
| } | ||
|
|
||
| Container.get(CredentialsOverwrites).setData(body); | ||
| await Container.get(CredentialsOverwrites).setData(body, true); |
There was a problem hiding this comment.
Avoid duplicate UI types regeneration. setData(body, true) already reloads the frontend by default, and calling frontendService.generateTypes() afterward redundantly repeats the operation. Pass reloadFrontend: false to setData here to prevent duplication.
Prompt for AI agents
Address the following comment on packages/cli/src/server.ts at line 318:
<comment>Avoid duplicate UI types regeneration. setData(body, true) already reloads the frontend by default, and calling frontendService.generateTypes() afterward redundantly repeats the operation. Pass reloadFrontend: false to setData here to prevent duplication.</comment>
<file context>
@@ -313,7 +315,7 @@ export class Server extends AbstractServer {
}
- Container.get(CredentialsOverwrites).setData(body);
+ await Container.get(CredentialsOverwrites).setData(body, true);
await frontendService?.generateTypes();
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
| export type ReloadLicense = ToCommand<'reload-license'>; | ||
| export type ReloadOIDCConfiguration = ToCommand<'reload-oidc-config'>; | ||
| export type ReloadSamlConfiguration = ToCommand<'reload-saml-config'>; | ||
| export type ReloadCredentialsOverrides = ToCommand<'reload-overwrite-credentials'>; |
There was a problem hiding this comment.
Alias name uses "Overrides" but the event key and feature terminology use "overwrite". Rename for consistency to avoid confusion.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Accurate that alias uses “Overrides” while the event key/feature use “overwrite”, confirmed by code. However, alias correctly maps to the event key, yielding no functional or type impact. This is a minor naming consistency issue and not worth reporting under medium sensitivity.
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/pubsub/pubsub.types.ts at line 45:
<comment>Alias name uses "Overrides" but the event key and feature terminology use "overwrite". Rename for consistency to avoid confusion.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Accurate that alias uses “Overrides” while the event key/feature use “overwrite”, confirmed by code. However, alias correctly maps to the event key, yielding no functional or type impact. This is a minor naming consistency issue and not worth reporting under medium sensitivity.</comment>
<file context>
@@ -42,6 +42,7 @@ export namespace PubSub {
export type ReloadLicense = ToCommand<'reload-license'>;
export type ReloadOIDCConfiguration = ToCommand<'reload-oidc-config'>;
export type ReloadSamlConfiguration = ToCommand<'reload-saml-config'>;
+ export type ReloadCredentialsOverrides = ToCommand<'reload-overwrite-credentials'>;
export type RestartEventBus = ToCommand<'restart-event-bus'>;
export type ReloadExternalSecretsProviders = ToCommand<'reload-external-secrets-providers'>;
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
| } | ||
|
|
||
| this.credentialsOverwrites.setData(req.body); | ||
| await this.credentialsOverwrites.setData(req.body, true); |
There was a problem hiding this comment.
Awaiting setData before setting the overwritesLoaded guard introduces a race: concurrent requests can bypass the guard and trigger duplicate writes and reloads. Consider setting the guard before the await or adding an internal mutex.
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/worker-server.ts at line 159:
<comment>Awaiting `setData` before setting the `overwritesLoaded` guard introduces a race: concurrent requests can bypass the guard and trigger duplicate writes and reloads. Consider setting the guard before the await or adding an internal mutex.</comment>
<file context>
@@ -156,7 +156,7 @@ export class WorkerServer {
}
- this.credentialsOverwrites.setData(req.body);
+ await this.credentialsOverwrites.setData(req.body, true);
this.overwritesLoaded = true;
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
| this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, (req, res) => | ||
| this.handleOverwrites(req, res), | ||
| ); | ||
| this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, async (req, res) => { |
There was a problem hiding this comment.
Async route lacks error handling; thrown/rejected errors from setData cause unhandled promise rejection and no error response in Express 4
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Express 5.1.0 is used; async handler errors are automatically forwarded to next(err) and produce a 500 response by default. The claim assumes Express 4 behavior, so it’s not applicable. No unhandled rejection or missing error response.
• Libraries consulted: Express 5 async route error handling, Expressjs-en-5x
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/worker-server.ts at line 123:
<comment>Async route lacks error handling; thrown/rejected errors from setData cause unhandled promise rejection and no error response in Express 4
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Express 5.1.0 is used; async handler errors are automatically forwarded to next(err) and produce a 500 response by default. The claim assumes Express 4 behavior, so it’s not applicable. No unhandled rejection or missing error response.
• **Libraries consulted**: Express 5 async route error handling, Expressjs-en-5x</comment>
<file context>
@@ -120,9 +120,9 @@ export class WorkerServer {
- this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, (req, res) =>
- this.handleOverwrites(req, res),
- );
+ this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, async (req, res) => {
+ await this.handleOverwrites(req, res);
+ });
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Functional Bugs Agent
| ); | ||
|
|
||
| // Mock a module import failure | ||
| jest.doMock('@/scaling/pubsub/publisher.service', () => { |
There was a problem hiding this comment.
jest.doMock is applied after the module was already imported in beforeEach, so the mock likely won't take effect due to module caching. This makes the test unreliable; reset/isolate modules before mocking or avoid the earlier import.
Prompt for AI agents
Address the following comment on packages/cli/src/__tests__/credentials-overwrites.test.ts at line 744:
<comment>jest.doMock is applied after the module was already imported in beforeEach, so the mock likely won't take effect due to module caching. This makes the test unreliable; reset/isolate modules before mocking or avoid the earlier import.</comment>
<file context>
@@ -234,4 +254,963 @@ describe('CredentialsOverwrites', () => {
+ );
+
+ // Mock a module import failure
+ jest.doMock('@/scaling/pubsub/publisher.service', () => {
+ throw new Error('Publisher service not available');
+ });
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
| expect(publisherMock.publishCommand).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should handle publisher import errors gracefully', async () => { |
There was a problem hiding this comment.
Test description/comment conflicts with assertion: it claims errors are handled gracefully (no throw) but expects a rejection. Align the description or the assertion to avoid misleading tests.
Prompt for AI agents
Address the following comment on packages/cli/src/__tests__/credentials-overwrites.test.ts at line 722:
<comment>Test description/comment conflicts with assertion: it claims errors are handled gracefully (no throw) but expects a rejection. Align the description or the assertion to avoid misleading tests.</comment>
<file context>
@@ -234,4 +254,963 @@ describe('CredentialsOverwrites', () => {
+ expect(publisherMock.publishCommand).not.toHaveBeenCalled();
+ });
+
+ it('should handle publisher import errors gracefully', async () => {
+ // Create a new config with persistence enabled
+ const errorConfig = mock<GlobalConfig>({
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
|
|
||
| await expect( | ||
| pubsubCredentialsOverwrites.reloadOverwriteCredentials(), | ||
| ).resolves.not.toThrow(); |
There was a problem hiding this comment.
Invalid Jest assertion: toThrow expects a function, not a resolved value. Replace with .resolves.toBeUndefined() to assert the async call does not reject.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Duplicate of #0 (same invalid .resolves.not.toThrow() pattern in PubSub tests). Covered by keeping #0.
• Libraries consulted: Jest expect resolves rejects toThrow matcher, Jestjs_io-docs-getting-started, Jest doMock module mocking caching resetModules isolateModules
Prompt for AI agents
Address the following comment on packages/cli/src/__tests__/credentials-overwrites.test.ts at line 611:
<comment>Invalid Jest assertion: toThrow expects a function, not a resolved value. Replace with .resolves.toBeUndefined() to assert the async call does not reject.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Duplicate of #0 (same invalid .resolves.not.toThrow() pattern in PubSub tests). Covered by keeping #0.
• **Libraries consulted**: Jest expect resolves rejects toThrow matcher, Jestjs_io-docs-getting-started, Jest doMock module mocking caching resetModules isolateModules</comment>
<file context>
@@ -234,4 +254,963 @@ describe('CredentialsOverwrites', () => {
+
+ await expect(
+ pubsubCredentialsOverwrites.reloadOverwriteCredentials(),
+ ).resolves.not.toThrow();
+
+ expect(settingsRepository.findByKey).toHaveBeenCalledWith('credentialsOverwrite');
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
| }); | ||
|
|
||
| // Should not throw but log error | ||
| await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow(); |
There was a problem hiding this comment.
Invalid Jest assertion: toThrow expects a function, but it's chained after .resolves on a promise. Use .resolves.toBeUndefined() (or assert a specific resolved value) to verify no rejection occurs.
Prompt for AI agents
Address the following comment on packages/cli/src/__tests__/credentials-overwrites.test.ts at line 445:
<comment>Invalid Jest assertion: toThrow expects a function, but it's chained after .resolves on a promise. Use .resolves.toBeUndefined() (or assert a specific resolved value) to verify no rejection occurs.</comment>
<file context>
@@ -234,4 +254,963 @@ describe('CredentialsOverwrites', () => {
+ });
+
+ // Should not throw but log error
+ await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow();
+
+ expect(settingsRepository.findByKey).toHaveBeenCalledWith('credentialsOverwrite');
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
There was a problem hiding this comment.
3 issues found across 12 files
Prompt for AI agents (all 3 issues)
Understand the root cause of the following 3 issues and fix them.
<file name="packages/@n8n/config/src/configs/credentials.config.ts">
<violation number="1" location="packages/@n8n/config/src/configs/credentials.config.ts:20">
The new `persistence` flag is documented as an authentication token, which misleads readers—please update the comment to describe the persistence toggle instead.</violation>
</file>
<file name="packages/cli/src/__tests__/credentials-overwrites.test.ts">
<violation number="1" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:744">
`jest.doMock` is called after the publisher service module has already been imported in this describe block, so the module is cached and the mock never takes effect. The dynamic import keeps returning the real publisher service, meaning this test cannot trigger the intended failure path and will fail incorrectly. Reset the module registry (e.g. `jest.resetModules()`) and re-import before mocking, or restructure the test so the mock is applied before the first import.</violation>
</file>
<file name="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts">
<violation number="1" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:329">
This time-based assertion makes the integration test flaky because sequential database writes can exceed two seconds under load. Please assert on observable behavior instead of a strict wall-clock limit.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| @Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN') | ||
| endpointAuthToken: string = ''; | ||
|
|
||
| /** Authentication token for the credentials overwrite endpoint. */ |
There was a problem hiding this comment.
The new persistence flag is documented as an authentication token, which misleads readers—please update the comment to describe the persistence toggle instead.
Prompt for AI agents
Address the following comment on packages/@n8n/config/src/configs/credentials.config.ts at line 20:
<comment>The new `persistence` flag is documented as an authentication token, which misleads readers—please update the comment to describe the persistence toggle instead.</comment>
<file context>
@@ -16,6 +16,10 @@ class CredentialsOverwrite {
@Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN')
endpointAuthToken: string = '';
+
+ /** Authentication token for the credentials overwrite endpoint. */
+ @Env('CREDENTIALS_OVERWRITE_PERSISTENCE')
+ persistence: boolean = false;
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
Suggested change
| /** Authentication token for the credentials overwrite endpoint. */ | |
| /** Whether credential overwrites should be persisted to the database. */ |
| ); | ||
|
|
||
| // Mock a module import failure | ||
| jest.doMock('@/scaling/pubsub/publisher.service', () => { |
There was a problem hiding this comment.
jest.doMock is called after the publisher service module has already been imported in this describe block, so the module is cached and the mock never takes effect. The dynamic import keeps returning the real publisher service, meaning this test cannot trigger the intended failure path and will fail incorrectly. Reset the module registry (e.g. jest.resetModules()) and re-import before mocking, or restructure the test so the mock is applied before the first import.
Prompt for AI agents
Address the following comment on packages/cli/src/__tests__/credentials-overwrites.test.ts at line 744:
<comment>`jest.doMock` is called after the publisher service module has already been imported in this describe block, so the module is cached and the mock never takes effect. The dynamic import keeps returning the real publisher service, meaning this test cannot trigger the intended failure path and will fail incorrectly. Reset the module registry (e.g. `jest.resetModules()`) and re-import before mocking, or restructure the test so the mock is applied before the first import.</comment>
<file context>
@@ -234,4 +254,963 @@ describe('CredentialsOverwrites', () => {
+ );
+
+ // Mock a module import failure
+ jest.doMock('@/scaling/pubsub/publisher.service', () => {
+ throw new Error('Publisher service not available');
+ });
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: General AI Review Agent
| const duration = Date.now() - startTime; | ||
|
|
||
| // Should complete in reasonable time (less than 2 seconds for 3 database operations) | ||
| expect(duration).toBeLessThan(2000); |
There was a problem hiding this comment.
This time-based assertion makes the integration test flaky because sequential database writes can exceed two seconds under load. Please assert on observable behavior instead of a strict wall-clock limit.
Prompt for AI agents
Address the following comment on packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts at line 329:
<comment>This time-based assertion makes the integration test flaky because sequential database writes can exceed two seconds under load. Please assert on observable behavior instead of a strict wall-clock limit.</comment>
<file context>
@@ -0,0 +1,503 @@
+ const duration = Date.now() - startTime;
+
+ // Should complete in reasonable time (less than 2 seconds for 3 database operations)
+ expect(duration).toBeLessThan(2000);
+
+ // Verify final state is the last operation
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
There was a problem hiding this comment.
2 issues found across 12 files
Prompt for AI agents (all 2 issues)
Understand the root cause of the following 2 issues and fix them.
<file name="packages/@n8n/config/src/configs/credentials.config.ts">
<violation number="1" location="packages/@n8n/config/src/configs/credentials.config.ts:20">
The documentation above `persistence` still describes an authentication token, which is misleading for anyone configuring this new boolean flag. Please update the comment to reflect that it enables persistence for credential overwrites.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*</violation>
</file>
<file name="packages/cli/src/server.ts">
<violation number="1" location="packages/cli/src/server.ts:305">
Invalid content-type check: req.contentType not standard and strict equality rejects valid JSON (e.g., with charset)</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| @Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN') | ||
| endpointAuthToken: string = ''; | ||
|
|
||
| /** Authentication token for the credentials overwrite endpoint. */ |
There was a problem hiding this comment.
The documentation above persistence still describes an authentication token, which is misleading for anyone configuring this new boolean flag. Please update the comment to reflect that it enables persistence for credential overwrites.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.
Prompt for AI agents
Address the following comment on packages/@n8n/config/src/configs/credentials.config.ts at line 20:
<comment>The documentation above `persistence` still describes an authentication token, which is misleading for anyone configuring this new boolean flag. Please update the comment to reflect that it enables persistence for credential overwrites.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*</comment>
<file context>
@@ -16,6 +16,10 @@ class CredentialsOverwrite {
@Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN')
endpointAuthToken: string = '';
+
+ /** Authentication token for the credentials overwrite endpoint. */
+ @Env('CREDENTIALS_OVERWRITE_PERSISTENCE')
+ persistence: boolean = false;
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
Suggested change
| /** Authentication token for the credentials overwrite endpoint. */ | |
| /** Enable persistence for credential overwrites. */ |
| async (req: express.Request, res: express.Response) => { | ||
| if (!this.presetCredentialsLoaded) { | ||
| // If authentication is enforced we can allow multiple overwrites | ||
| if (!this.presetCredentialsLoaded || authenticationEnforced) { |
There was a problem hiding this comment.
Invalid content-type check: req.contentType not standard and strict equality rejects valid JSON (e.g., with charset)
Prompt for AI agents
Address the following comment on packages/cli/src/server.ts at line 305:
<comment>Invalid content-type check: req.contentType not standard and strict equality rejects valid JSON (e.g., with charset)</comment>
<file context>
@@ -297,10 +297,12 @@ export class Server extends AbstractServer {
async (req: express.Request, res: express.Response) => {
- if (!this.presetCredentialsLoaded) {
+ // If authentication is enforced we can allow multiple overwrites
+ if (!this.presetCredentialsLoaded || authenticationEnforced) {
const body = req.body as ICredentialsOverwrite;
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: Functional Bugs Agent
There was a problem hiding this comment.
4 issues found across 12 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="packages/@n8n/config/src/configs/credentials.config.ts">
<violation number="1" location="packages/@n8n/config/src/configs/credentials.config.ts:20">
The new `persistence` flag is documented as another authentication token, which misleads anyone configuring or reviewing the setting. Please update the comment to reflect that this boolean controls credential overwrite persistence.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*</violation>
</file>
<file name="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts">
<violation number="1" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:329">
Please avoid asserting a strict `<2s` wall-clock duration for these DB operations; the integration test will be flaky on slower machines. Consider removing the timing assertion or switching to a deterministic condition instead.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*</violation>
</file>
<file name="packages/cli/src/__tests__/credentials-overwrites.test.ts">
<violation number="1" location="packages/cli/src/__tests__/credentials-overwrites.test.ts:445">
Jest's `toThrow` matcher can't be used with `.resolves`; the promise resolves to `undefined`, so the matcher itself throws "Received value must be a function", breaking the test. Replace it with an assertion that the promise resolves (e.g. `.resolves.toBeUndefined()`), and update the other occurrences in this block similarly.</violation>
</file>
<file name="packages/cli/src/scaling/worker-server.ts">
<violation number="1" location="packages/cli/src/scaling/worker-server.ts:124">
Async Express route awaits without try/catch; rejections from handleOverwrites (e.g. DB write in setData) will be unhandled in Express 4.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Invalid. The repo uses Express 5.1.0 (pnpm-lock), which propagates async handler rejections to the error handler by default. The Express 4 concern about unhandled rejections doesn’t apply.
• **Libraries consulted**: Express 5 async handlers error handling promises, Express.js, Express</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| @Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN') | ||
| endpointAuthToken: string = ''; | ||
|
|
||
| /** Authentication token for the credentials overwrite endpoint. */ |
There was a problem hiding this comment.
The new persistence flag is documented as another authentication token, which misleads anyone configuring or reviewing the setting. Please update the comment to reflect that this boolean controls credential overwrite persistence.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.
Prompt for AI agents
Address the following comment on packages/@n8n/config/src/configs/credentials.config.ts at line 20:
<comment>The new `persistence` flag is documented as another authentication token, which misleads anyone configuring or reviewing the setting. Please update the comment to reflect that this boolean controls credential overwrite persistence.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: typescriptUnderstanding.*</comment>
<file context>
@@ -16,6 +16,10 @@ class CredentialsOverwrite {
@Env('CREDENTIALS_OVERWRITE_ENDPOINT_AUTH_TOKEN')
endpointAuthToken: string = '';
+
+ /** Authentication token for the credentials overwrite endpoint. */
+ @Env('CREDENTIALS_OVERWRITE_PERSISTENCE')
+ persistence: boolean = false;
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
Suggested change
| /** Authentication token for the credentials overwrite endpoint. */ | |
| /** Enables persistence of credential overwrites in the database. */ |
| const duration = Date.now() - startTime; | ||
|
|
||
| // Should complete in reasonable time (less than 2 seconds for 3 database operations) | ||
| expect(duration).toBeLessThan(2000); |
There was a problem hiding this comment.
Please avoid asserting a strict <2s wall-clock duration for these DB operations; the integration test will be flaky on slower machines. Consider removing the timing assertion or switching to a deterministic condition instead.
DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.
Prompt for AI agents
Address the following comment on packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts at line 329:
<comment>Please avoid asserting a strict `<2s` wall-clock duration for these DB operations; the integration test will be flaky on slower machines. Consider removing the timing assertion or switching to a deterministic condition instead.
*DEV MODE: This violation would have been filtered out by screening filters. Failing filters: functionalImpact.*</comment>
<file context>
@@ -0,0 +1,503 @@
+ const duration = Date.now() - startTime;
+
+ // Should complete in reasonable time (less than 2 seconds for 3 database operations)
+ expect(duration).toBeLessThan(2000);
+
+ // Verify final state is the last operation
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
| }); | ||
|
|
||
| // Should not throw but log error | ||
| await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow(); |
There was a problem hiding this comment.
Jest's toThrow matcher can't be used with .resolves; the promise resolves to undefined, so the matcher itself throws "Received value must be a function", breaking the test. Replace it with an assertion that the promise resolves (e.g. .resolves.toBeUndefined()), and update the other occurrences in this block similarly.
Prompt for AI agents
Address the following comment on packages/cli/src/__tests__/credentials-overwrites.test.ts at line 445:
<comment>Jest's `toThrow` matcher can't be used with `.resolves`; the promise resolves to `undefined`, so the matcher itself throws "Received value must be a function", breaking the test. Replace it with an assertion that the promise resolves (e.g. `.resolves.toBeUndefined()`), and update the other occurrences in this block similarly.</comment>
<file context>
@@ -234,4 +254,963 @@ describe('CredentialsOverwrites', () => {
+ });
+
+ // Should not throw but log error
+ await expect(dbCredentialsOverwrites.loadOverwriteDataFromDB(false)).resolves.not.toThrow();
+
+ expect(settingsRepository.findByKey).toHaveBeenCalledWith('credentialsOverwrite');
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: General AI Review Agent
| this.handleOverwrites(req, res), | ||
| ); | ||
| this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, async (req, res) => { | ||
| await this.handleOverwrites(req, res); |
There was a problem hiding this comment.
Async Express route awaits without try/catch; rejections from handleOverwrites (e.g. DB write in setData) will be unhandled in Express 4.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: Invalid. The repo uses Express 5.1.0 (pnpm-lock), which propagates async handler rejections to the error handler by default. The Express 4 concern about unhandled rejections doesn’t apply.
• Libraries consulted: Express 5 async handlers error handling promises, Express.js, Express
Prompt for AI agents
Address the following comment on packages/cli/src/scaling/worker-server.ts at line 124:
<comment>Async Express route awaits without try/catch; rejections from handleOverwrites (e.g. DB write in setData) will be unhandled in Express 4.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: Invalid. The repo uses Express 5.1.0 (pnpm-lock), which propagates async handler rejections to the error handler by default. The Express 4 concern about unhandled rejections doesn’t apply.
• **Libraries consulted**: Express 5 async handlers error handling promises, Express.js, Express</comment>
<file context>
@@ -120,9 +120,9 @@ export class WorkerServer {
- this.handleOverwrites(req, res),
- );
+ this.app.post(`/${endpoint}`, rawBodyReader, bodyParser, async (req, res) => {
+ await this.handleOverwrites(req, res);
+ });
}
</file context>
[internal] Confidence score: 9/10
[internal] Posted by: Functional Bugs Agent
There was a problem hiding this comment.
4 issues found across 12 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="packages/cli/src/credentials-overwrites.ts">
<violation number="1" location="packages/cli/src/credentials-overwrites.ts:57">
Returning early when a reload is already running drops the newer reload request, so a later update can be skipped entirely and leave this instance with stale credential overwrites. Consider queuing or awaiting the in-flight reload instead of exiting early.</violation>
<violation number="2" location="packages/cli/src/credentials-overwrites.ts:65">
Prototype Pollution: Untrusted JSON from DB is parsed and later merged with Object.assign and direct key assignment, allowing "__proto__" keys to modify object prototypes.</violation>
</file>
<file name="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts">
<violation number="1" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:29">
This test rewrites the shared GlobalConfig without restoring it, so later suites will run with these test-only credentials settings. Please keep the original config and restore it in `afterAll` to avoid leaking state across tests.</violation>
<violation number="2" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:329">
Hard-coding a 2s upper bound for this database flow makes the test fragile; on slower CI nodes it may fail despite working code. Please remove the strict timing assertion or replace it with a more robust check.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The 2-second bound is a deliberate performance assertion in the new test. There is no evidence it is too tight or causing failures, so labeling it as flaky would be speculative.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| } | ||
|
|
||
| async loadOverwriteDataFromDB(reloadFrontend: boolean) { | ||
| if (this.reloading) return; |
There was a problem hiding this comment.
Returning early when a reload is already running drops the newer reload request, so a later update can be skipped entirely and leave this instance with stale credential overwrites. Consider queuing or awaiting the in-flight reload instead of exiting early.
Prompt for AI agents
Address the following comment on packages/cli/src/credentials-overwrites.ts at line 57:
<comment>Returning early when a reload is already running drops the newer reload request, so a later update can be skipped entirely and leave this instance with stale credential overwrites. Consider queuing or awaiting the in-flight reload instead of exiting early.</comment>
<file context>
@@ -18,13 +23,78 @@ export class CredentialsOverwrites {
+ }
+
+ async loadOverwriteDataFromDB(reloadFrontend: boolean) {
+ if (this.reloading) return;
+ try {
+ this.reloading = true;
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: General AI Review Agent
| const duration = Date.now() - startTime; | ||
|
|
||
| // Should complete in reasonable time (less than 2 seconds for 3 database operations) | ||
| expect(duration).toBeLessThan(2000); |
There was a problem hiding this comment.
Hard-coding a 2s upper bound for this database flow makes the test fragile; on slower CI nodes it may fail despite working code. Please remove the strict timing assertion or replace it with a more robust check.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The 2-second bound is a deliberate performance assertion in the new test. There is no evidence it is too tight or causing failures, so labeling it as flaky would be speculative.
Prompt for AI agents
Address the following comment on packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts at line 329:
<comment>Hard-coding a 2s upper bound for this database flow makes the test fragile; on slower CI nodes it may fail despite working code. Please remove the strict timing assertion or replace it with a more robust check.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The 2-second bound is a deliberate performance assertion in the new test. There is no evidence it is too tight or causing failures, so labeling it as flaky would be speculative.</comment>
<file context>
@@ -0,0 +1,503 @@
+ const duration = Date.now() - startTime;
+
+ // Should complete in reasonable time (less than 2 seconds for 3 database operations)
+ expect(duration).toBeLessThan(2000);
+
+ // Verify final state is the last operation
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: General AI Review Agent
| credentialTypes = Container.get(CredentialTypes); | ||
|
|
||
| // Configure globalConfig for integration tests | ||
| (globalConfig as any).credentials = { |
There was a problem hiding this comment.
This test rewrites the shared GlobalConfig without restoring it, so later suites will run with these test-only credentials settings. Please keep the original config and restore it in afterAll to avoid leaking state across tests.
Prompt for AI agents
Address the following comment on packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts at line 29:
<comment>This test rewrites the shared GlobalConfig without restoring it, so later suites will run with these test-only credentials settings. Please keep the original config and restore it in `afterAll` to avoid leaking state across tests.</comment>
<file context>
@@ -0,0 +1,503 @@
+ credentialTypes = Container.get(CredentialTypes);
+
+ // Configure globalConfig for integration tests
+ (globalConfig as any).credentials = {
+ overwrite: {
+ data: '', // No static data for integration tests
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
|
|
||
| if (data) { | ||
| const decryptedData = this.cipher.decrypt(data.value); | ||
| const overwriteData = jsonParse<ICredentialsOverwrite>(decryptedData, { |
There was a problem hiding this comment.
Prototype Pollution: Untrusted JSON from DB is parsed and later merged with Object.assign and direct key assignment, allowing "proto" keys to modify object prototypes.
Prompt for AI agents
Address the following comment on packages/cli/src/credentials-overwrites.ts at line 65:
<comment>Prototype Pollution: Untrusted JSON from DB is parsed and later merged with Object.assign and direct key assignment, allowing "__proto__" keys to modify object prototypes.</comment>
<file context>
@@ -18,13 +23,78 @@ export class CredentialsOverwrites {
+
+ if (data) {
+ const decryptedData = this.cipher.decrypt(data.value);
+ const overwriteData = jsonParse<ICredentialsOverwrite>(decryptedData, {
+ errorMessage: 'The credentials-overwrite is not valid JSON.',
+ });
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: Security Vulnerabilities Agent
There was a problem hiding this comment.
4 issues found across 12 files
Prompt for AI agents (all 4 issues)
Understand the root cause of the following 4 issues and fix them.
<file name="packages/cli/src/credentials-overwrites.ts">
<violation number="1" location="packages/cli/src/credentials-overwrites.ts:57">
Returning early when a reload is already running drops the newer reload request, so a later update can be skipped entirely and leave this instance with stale credential overwrites. Consider queuing or awaiting the in-flight reload instead of exiting early.</violation>
<violation number="2" location="packages/cli/src/credentials-overwrites.ts:65">
Prototype Pollution: Untrusted JSON from DB is parsed and later merged with Object.assign and direct key assignment, allowing "__proto__" keys to modify object prototypes.</violation>
</file>
<file name="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts">
<violation number="1" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:29">
This test rewrites the shared GlobalConfig without restoring it, so later suites will run with these test-only credentials settings. Please keep the original config and restore it in `afterAll` to avoid leaking state across tests.</violation>
<violation number="2" location="packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts:329">
Hard-coding a 2s upper bound for this database flow makes the test fragile; on slower CI nodes it may fail despite working code. Please remove the strict timing assertion or replace it with a more robust check.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The 2-second bound is a deliberate performance assertion in the new test. There is no evidence it is too tight or causing failures, so labeling it as flaky would be speculative.</violation>
</file>
React with 👍 or 👎 to teach cubic. Mention @cubic-dev-ai to give feedback, ask questions, or re-run the review.
| } | ||
|
|
||
| async loadOverwriteDataFromDB(reloadFrontend: boolean) { | ||
| if (this.reloading) return; |
There was a problem hiding this comment.
Returning early when a reload is already running drops the newer reload request, so a later update can be skipped entirely and leave this instance with stale credential overwrites. Consider queuing or awaiting the in-flight reload instead of exiting early.
Prompt for AI agents
Address the following comment on packages/cli/src/credentials-overwrites.ts at line 57:
<comment>Returning early when a reload is already running drops the newer reload request, so a later update can be skipped entirely and leave this instance with stale credential overwrites. Consider queuing or awaiting the in-flight reload instead of exiting early.</comment>
<file context>
@@ -18,13 +23,78 @@ export class CredentialsOverwrites {
+ }
+
+ async loadOverwriteDataFromDB(reloadFrontend: boolean) {
+ if (this.reloading) return;
+ try {
+ this.reloading = true;
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: General AI Review Agent
| const duration = Date.now() - startTime; | ||
|
|
||
| // Should complete in reasonable time (less than 2 seconds for 3 database operations) | ||
| expect(duration).toBeLessThan(2000); |
There was a problem hiding this comment.
Hard-coding a 2s upper bound for this database flow makes the test fragile; on slower CI nodes it may fail despite working code. Please remove the strict timing assertion or replace it with a more robust check.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• GPT-5: The 2-second bound is a deliberate performance assertion in the new test. There is no evidence it is too tight or causing failures, so labeling it as flaky would be speculative.
Prompt for AI agents
Address the following comment on packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts at line 329:
<comment>Hard-coding a 2s upper bound for this database flow makes the test fragile; on slower CI nodes it may fail despite working code. Please remove the strict timing assertion or replace it with a more robust check.
DEV MODE: This violation would have been filtered out by GPT-5.
Reasoning:
• **GPT-5**: The 2-second bound is a deliberate performance assertion in the new test. There is no evidence it is too tight or causing failures, so labeling it as flaky would be speculative.</comment>
<file context>
@@ -0,0 +1,503 @@
+ const duration = Date.now() - startTime;
+
+ // Should complete in reasonable time (less than 2 seconds for 3 database operations)
+ expect(duration).toBeLessThan(2000);
+
+ // Verify final state is the last operation
</file context>
[internal] Confidence score: 7/10
[internal] Posted by: General AI Review Agent
| credentialTypes = Container.get(CredentialTypes); | ||
|
|
||
| // Configure globalConfig for integration tests | ||
| (globalConfig as any).credentials = { |
There was a problem hiding this comment.
This test rewrites the shared GlobalConfig without restoring it, so later suites will run with these test-only credentials settings. Please keep the original config and restore it in afterAll to avoid leaking state across tests.
Prompt for AI agents
Address the following comment on packages/cli/test/integration/credentials/credentials-overwrites.integration.test.ts at line 29:
<comment>This test rewrites the shared GlobalConfig without restoring it, so later suites will run with these test-only credentials settings. Please keep the original config and restore it in `afterAll` to avoid leaking state across tests.</comment>
<file context>
@@ -0,0 +1,503 @@
+ credentialTypes = Container.get(CredentialTypes);
+
+ // Configure globalConfig for integration tests
+ (globalConfig as any).credentials = {
+ overwrite: {
+ data: '', // No static data for integration tests
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: General AI Review Agent
|
|
||
| if (data) { | ||
| const decryptedData = this.cipher.decrypt(data.value); | ||
| const overwriteData = jsonParse<ICredentialsOverwrite>(decryptedData, { |
There was a problem hiding this comment.
Prototype Pollution: Untrusted JSON from DB is parsed and later merged with Object.assign and direct key assignment, allowing "proto" keys to modify object prototypes.
Prompt for AI agents
Address the following comment on packages/cli/src/credentials-overwrites.ts at line 65:
<comment>Prototype Pollution: Untrusted JSON from DB is parsed and later merged with Object.assign and direct key assignment, allowing "__proto__" keys to modify object prototypes.</comment>
<file context>
@@ -18,13 +23,78 @@ export class CredentialsOverwrites {
+
+ if (data) {
+ const decryptedData = this.cipher.decrypt(data.value);
+ const overwriteData = jsonParse<ICredentialsOverwrite>(decryptedData, {
+ errorMessage: 'The credentials-overwrite is not valid JSON.',
+ });
</file context>
[internal] Confidence score: 8/10
[internal] Posted by: Security Vulnerabilities Agent
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Credential overwrites are stored in memory, if they are set using the REST API, each instance need to receive the same overwrites. In a large scale system, this means tracking which pod has his credentials overwritten already, on restarts these have to be overwritten again. This allows users to configure persistence for credential overwrites, by storing them encrypted in the database and adding synchronization via the event system for loading it from the database.
Related Linear tickets, Github issues, and Community forum posts
closes https://linear.app/n8n/issue/PAY-3786/add-support-for-storing-credential-overwrites-in-the-settings-table
closes https://linear.app/n8n/issue/PAY-3761/customer-feedback-credential-overwrite-feature-lacking-security
Review / Merge checklist
PR title and summary are descriptive. (conventions)
Docs updated or follow-up ticket created.
Tests included.
PR Labeled with
release/backport(if the PR is an urgent fix that needs to be backported)Based on: chore(core): Add support for persisting and synchronizing credential overwrites n8n-io/n8n#19919
Summary by cubic
Adds encrypted persistence and cluster-wide sync for credential overwrites. Overwrites can now be stored in the DB, auto-loaded on startup, and synced across pods via Pub/Sub with a UI refresh. Addresses Linear PAY-3786 and PAY-3761.