feat(@deepnote/cli): support linting integrations env yaml file directly#337
Draft
feat(@deepnote/cli): support linting integrations env yaml file directly#337
Conversation
Allow `deepnote lint .deepnote.env.yaml` to validate an integrations file without requiring a .deepnote notebook. Validates YAML structure, integration schemas, and env var references, displaying human-readable errors consistent with the existing configuration issues output. Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Contributor
📝 WalkthroughWalkthroughThis change extends the lint command to support linting integrations YAML files directly, in addition to .deepnote notebooks. The CLI argument becomes optional with a new Sequence DiagramsequenceDiagram
participant User
participant CLI as lint command
participant FileDetector as File Type Detector
participant NotebookLinter as Notebook Linter
participant IntegrationsLinter as Integrations Linter
participant EnvLoader as Environment Loader
participant OutputFormatter as Output Formatter
User->>CLI: lint [path] --integrations-file [opt-path]
activate CLI
CLI->>FileDetector: Detect file type (YAML or notebook)
activate FileDetector
FileDetector-->>CLI: File type result
deactivate FileDetector
alt YAML path provided
CLI->>IntegrationsLinter: lintIntegrationsFile(path)
activate IntegrationsLinter
IntegrationsLinter->>IntegrationsLinter: Parse YAML & validate schema
IntegrationsLinter-->>CLI: Issues found
deactivate IntegrationsLinter
else Notebook path provided
CLI->>EnvLoader: Load .env & integrations file
activate EnvLoader
EnvLoader-->>CLI: Environment variables
deactivate EnvLoader
CLI->>NotebookLinter: Lint notebook with env vars
activate NotebookLinter
NotebookLinter-->>CLI: Notebook issues
deactivate NotebookLinter
CLI->>IntegrationsLinter: Lint integrations file
activate IntegrationsLinter
IntegrationsLinter-->>CLI: Integrations issues
deactivate IntegrationsLinter
end
CLI->>OutputFormatter: Format aggregated results
activate OutputFormatter
OutputFormatter-->>CLI: Formatted output
deactivate OutputFormatter
CLI-->>User: Exit with code
deactivate CLI
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/cli/src/commands/lint.test.ts`:
- Around line 774-793: The test for createLintAction in lint.test.ts only
asserts integrationsFile.issues and exit behavior and misses verifying the
top-level JSON contract on failures; update the failing-case tests (e.g., the
'outputs JSON with issues for an invalid integrations yaml file' case that calls
action(intFile, { output: 'json' })) to also assert parsed.success is false and
parsed.issueCount.errors > 0 and parsed.issueCount.total > 0 (and add a similar
assertion in one notebook+integrations failure test) so configuration failures
cannot serialize as success: true or zero issue counts; locate and modify the
tests that call createLintAction/action and parse JSON output to include these
additional assertions.
- Around line 1242-1257: The test currently assumes a missing user-specified
integrations file silently yields an empty config; instead, update the CLI's
lint handling so that when the integrationsFile option is provided but the file
does not exist you surface an error and exit with invalid-usage semantics;
locate the lint option parsing / handler (createLintAction and the lint command
logic in packages/cli/src/commands/lint.ts) and change the branch that reads
integrationsFile to validate existence and throw or call the existing error/exit
path (same style as the direct YAML path handling around the existing error
block) rather than falling back to an empty integrations result.
In `@packages/cli/src/commands/lint.ts`:
- Around line 112-120: The returned lint status currently spreads the result of
lintFile() and embeds a hardcoded integrationsFile object from
lintIntegrationsFile() which sets zero issue counts, so invalid integrations
produce inconsistent success/issueCount; update
lintIntegrationsFile(integrationsFilePath, parsedIntegrations) to compute and
return true issue counts (issueCount.total and per-category counts) and a
success boolean based on parsedIntegrations.issues and
parsedIntegrations.integrations parsing results, and then merge those counts
into the top-level lint object returned by the function that builds the final
result (the block returning { path: absolutePath, ...lint, integrationsFile: {
... } }) so that overall issueCount and success reflect both lintFile() and
lintIntegrationsFile() outcomes. Ensure you reference lintFile(),
lintIntegrationsFile(), parsedIntegrations and the returned integrationsFile
fields when making the change.
- Around line 82-103: The lint command currently mutates process.env by calling
dotenv.config(...) and injecting envVars from
getEnvironmentVariablesForIntegrations(parsedIntegrations.integrations, ...)
without restoring originals, causing subsequent lint runs in the same process to
see stale/overwritten values; fix by saving the original environment state (for
each env var you will set and the result of dotenv.config), apply the dotenv and
injected envVars only for the duration of the lint run (the logic around
parseIntegrationsFile / getEnvironmentVariablesForIntegrations and subsequent
checks), and restore process.env to its prior values after the lint completes or
on error (ensure restoration covers both newly added vars and overwritten values
so caller-provided envs are preserved); use the function/variable names
parseIntegrationsFile, getEnvironmentVariablesForIntegrations, dotenv.config,
and the envVars loop to locate where to capture and later restore the original
environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d0f233c3-5bbc-433a-9b95-4b488f62d7e7
📒 Files selected for processing (3)
packages/cli/src/cli.tspackages/cli/src/commands/lint.test.tspackages/cli/src/commands/lint.ts
Comment on lines
+774
to
+793
| it('outputs JSON with issues for an invalid integrations yaml file', async () => { | ||
| const action = createLintAction(program) | ||
| const intFile = join(tempDir, 'invalid-json.yaml') | ||
| await writeFile( | ||
| intFile, | ||
| `integrations: | ||
| - id: bad | ||
| name: Bad | ||
| type: invalid-type | ||
| metadata: {}` | ||
| ) | ||
|
|
||
| exitSpy.mockRestore() | ||
| exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never) | ||
|
|
||
| await action(intFile, { output: 'json' }) | ||
|
|
||
| const out = getOutput(consoleSpy) | ||
| const parsed = JSON.parse(out) | ||
| expect(parsed.integrationsFile.issues.length).toBeGreaterThan(0) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Assert the top-level JSON contract in the failing cases.
These tests only check integrationsFile.issues or the exit code. They won't catch the regression where configuration failures still serialize with success: true and issueCount.total === 0. Add assertions for success, issueCount.errors, and issueCount.total in at least one notebook+integrations failure and one direct-YAML failure.
As per coding guidelines, **/*.test.{ts,tsx}: Create comprehensive tests for all new features using Vitest, and Test edge cases, error handling, and special characters in test files.
Also applies to: 944-985, 1118-1141
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/lint.test.ts` around lines 774 - 793, The test for
createLintAction in lint.test.ts only asserts integrationsFile.issues and exit
behavior and misses verifying the top-level JSON contract on failures; update
the failing-case tests (e.g., the 'outputs JSON with issues for an invalid
integrations yaml file' case that calls action(intFile, { output: 'json' })) to
also assert parsed.success is false and parsed.issueCount.errors > 0 and
parsed.issueCount.total > 0 (and add a similar assertion in one
notebook+integrations failure test) so configuration failures cannot serialize
as success: true or zero issue counts; locate and modify the tests that call
createLintAction/action and parse JSON output to include these additional
assertions.
Comment on lines
+1242
to
+1257
| it('returns empty result when custom integrations file does not exist', async () => { | ||
| const action = createLintAction(program) | ||
| const filePath = resolve(process.cwd(), HELLO_WORLD_FILE) | ||
| const nonExistentFile = join(tempDir, 'does-not-exist.yaml') | ||
|
|
||
| exitSpy.mockRestore() | ||
| exitSpy = vi.spyOn(process, 'exit').mockImplementation(() => undefined as never) | ||
|
|
||
| await action(filePath, { output: 'json', integrationsFile: nonExistentFile }) | ||
|
|
||
| const out = getOutput(consoleSpy) | ||
| const parsed = JSON.parse(out) | ||
|
|
||
| expect(parsed.integrationsFile.integrationCount).toBe(0) | ||
| expect(parsed.integrationsFile.issues).toHaveLength(0) | ||
| }) |
Contributor
There was a problem hiding this comment.
Don't treat a typoed --integrations-file as an empty config.
This test bakes in silent fallback for an explicit override. A missing user-supplied integrations file should fail fast with invalid-usage semantics; otherwise a typo changes lint behavior instead of surfacing the bad path. That would also align with packages/cli/src/commands/lint.ts Lines 130-138, which already error on a missing direct YAML path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/lint.test.ts` around lines 1242 - 1257, The test
currently assumes a missing user-specified integrations file silently yields an
empty config; instead, update the CLI's lint handling so that when the
integrationsFile option is provided but the file does not exist you surface an
error and exit with invalid-usage semantics; locate the lint option parsing /
handler (createLintAction and the lint command logic in
packages/cli/src/commands/lint.ts) and change the branch that reads
integrationsFile to validate existence and throw or call the existing error/exit
path (same style as the direct YAML path handling around the existing error
block) rather than falling back to an empty integrations result.
Comment on lines
+82
to
+103
| // Load .env file so env: references in integrations can be resolved | ||
| dotenv.config({ path: join(fileDir, DEFAULT_ENV_FILE), quiet: true }) | ||
|
|
||
| // Load and parse integrations file | ||
| const integrationsFilePath = options.integrationsFile ?? getDefaultIntegrationsFilePath(fileDir) | ||
| debug(`Loading integrations from: ${integrationsFilePath}`) | ||
| const parsedIntegrations = await parseIntegrationsFile(integrationsFilePath) | ||
|
|
||
| debug(`Parsed ${parsedIntegrations.integrations.length} integrations, ${parsedIntegrations.issues.length} issues`) | ||
|
|
||
| // Inject integration env vars into process.env so checkMissingIntegrations works correctly | ||
| if (parsedIntegrations.integrations.length > 0) { | ||
| const { envVars, errors } = getEnvironmentVariablesForIntegrations(parsedIntegrations.integrations, { | ||
| projectRootDirectory: fileDir, | ||
| }) | ||
| for (const err of errors) { | ||
| debug(`Integration env var error: ${err.message}`) | ||
| } | ||
| for (const { name, value } of envVars) { | ||
| process.env[name] = value | ||
| } | ||
| debug(`Injected ${envVars.length} environment variables for ${parsedIntegrations.integrations.length} integrations`) |
Contributor
There was a problem hiding this comment.
Scope .env loading and injected integration vars to a single lint run.
Both code paths mutate process.env and never restore it. After one lint, later invocations in the same process can stop reporting env_var_not_defined/missing-integration, and caller-provided env values stay overwritten.
Also applies to: 142-143
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/lint.ts` around lines 82 - 103, The lint command
currently mutates process.env by calling dotenv.config(...) and injecting
envVars from
getEnvironmentVariablesForIntegrations(parsedIntegrations.integrations, ...)
without restoring originals, causing subsequent lint runs in the same process to
see stale/overwritten values; fix by saving the original environment state (for
each env var you will set and the result of dotenv.config), apply the dotenv and
injected envVars only for the duration of the lint run (the logic around
parseIntegrationsFile / getEnvironmentVariablesForIntegrations and subsequent
checks), and restore process.env to its prior values after the lint completes or
on error (ensure restoration covers both newly added vars and overwritten values
so caller-provided envs are preserved); use the function/variable names
parseIntegrationsFile, getEnvironmentVariablesForIntegrations, dotenv.config,
and the envVars loop to locate where to capture and later restore the original
environment.
Comment on lines
112
to
+120
| return { | ||
| path: absolutePath, | ||
| ...lint, | ||
| integrationsFile: { | ||
| path: integrationsFilePath, | ||
| integrationCount: parsedIntegrations.integrations.length, | ||
| issues: parsedIntegrations.issues, | ||
| }, | ||
| } |
Contributor
There was a problem hiding this comment.
Count configuration failures in the returned lint status.
A clean notebook plus an invalid integrations file currently serializes as success: true and issueCount.total === 0, because lintFile() just spreads lint and lintIntegrationsFile() hardcodes zero counts. The CLI still exits 1, so JSON consumers get contradictory results.
Suggested fix
+ const configurationErrorCount = parsedIntegrations.issues.length
return {
path: absolutePath,
...lint,
+ success: lint.success && configurationErrorCount === 0,
+ issueCount: {
+ errors: lint.issueCount.errors + configurationErrorCount,
+ warnings: lint.issueCount.warnings,
+ total: lint.issueCount.total + configurationErrorCount,
+ },
integrationsFile: {
path: integrationsFilePath,
integrationCount: parsedIntegrations.integrations.length,
issues: parsedIntegrations.issues,
},
}- const hasErrors = parsedIntegrations.issues.length > 0
+ const configurationErrorCount = parsedIntegrations.issues.length
return {
path: absolutePath,
- success: !hasErrors,
- issueCount: { errors: 0, warnings: 0, total: 0 },
+ success: configurationErrorCount === 0,
+ issueCount: {
+ errors: configurationErrorCount,
+ warnings: 0,
+ total: configurationErrorCount,
+ },
issues: [],Also applies to: 150-160
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/cli/src/commands/lint.ts` around lines 112 - 120, The returned lint
status currently spreads the result of lintFile() and embeds a hardcoded
integrationsFile object from lintIntegrationsFile() which sets zero issue
counts, so invalid integrations produce inconsistent success/issueCount; update
lintIntegrationsFile(integrationsFilePath, parsedIntegrations) to compute and
return true issue counts (issueCount.total and per-category counts) and a
success boolean based on parsedIntegrations.issues and
parsedIntegrations.integrations parsing results, and then merge those counts
into the top-level lint object returned by the function that builds the final
result (the block returning { path: absolutePath, ...lint, integrationsFile: {
... } }) so that overall issueCount and success reflect both lintFile() and
lintIntegrationsFile() outcomes. Ensure you reference lintFile(),
lintIntegrationsFile(), parsedIntegrations and the returned integrationsFile
fields when making the change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Allow
deepnote lint .deepnote.env.yamlto validate an integrationsfile without requiring a .deepnote notebook. Validates YAML structure,
integration schemas, and env var references, displaying human-readable
errors consistent with the existing configuration issues output.
Co-Authored-By: Claude Sonnet 4.6 [email protected]
Summary by CodeRabbit
lintcommand now validates integrations YAML files directly, in addition to .deepnote files--integrations-fileoption to specify a custom integrations file path for linting