Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions src/commands/deploy/deploy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -716,6 +716,19 @@ const handleBuild = async ({
})

const { configMutations, exitCode, newConfig, logs } = await runBuild(resolvedOptions)

// When --verbose is used with --json, pipe the build's stdout and stderr
// to the process stderr so that callers can see the full build output
// while still getting clean JSON on stdout.
Comment on lines +720 to +722
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Remove explanatory comment per coding guidelines.

The comment describes what the code does rather than why. The function and flag names are self-documenting. As per coding guidelines: "Never write comments on what the code does; make the code clean and self-explanatory instead."

♻️ Proposed fix
   const { configMutations, exitCode, newConfig, logs } = await runBuild(resolvedOptions)

-  // When --verbose is used with --json, pipe the build's stdout and stderr
-  // to the process stderr so that callers can see the full build output
-  // while still getting clean JSON on stdout.
   if (options.verbose && options.json && logs) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/deploy/deploy.ts` around lines 720 - 722, Remove the explanatory
comment that describes what the code does around the logic that pipes build
stdout/stderr to process.stderr when both the --verbose and --json flags are
set; instead rely on the self-documenting flag and function names (e.g., the
verbose/json flag handling and the code that writes to process.stderr) so delete
the three-line comment block immediately above the piping logic in deploy.ts
without changing the surrounding implementation.

if (options.verbose && options.json && logs) {
if (logs.stdout.length) {
process.stderr.write(logs.stdout.join('\n'))
}
if (logs.stderr.length) {
process.stderr.write(logs.stderr.join('\n'))
}
}
Comment on lines +723 to +730
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

🧩 Analysis chain

🌐 Web query:

What is the Logs type structure in @netlify/build package?

💡 Result:

The @netlify/build package does not expose a publicly documented "Logs" type structure in its API. The package is the core Netlify Build engine that orchestrates build commands, plugins, and function bundling during deploys. From source code analysis in the netlify/build GitHub monorepo (packages/build), logging is handled internally through modules like src/log/ (inferred from PRs mentioning log verbosity, system logs, and error logging). Build plugins receive logging capabilities via the utils object in event handlers, where console.log outputs appear in build logs, and utils.status.show displays deploy summary info. No explicit "Logs" interface or type is exported for plugin authors in the TypeScript definitions. npm package docs confirm no README details on types, and official Netlify docs focus on plugin utils for logging rather than a dedicated Logs structure. Recent PRs (e.g., #5643, #5679, #6156) discuss internal log handling like output flushing, system logs for plugin failures, and verbosity reduction, but no public Logs type. If referring to internal types, error types were typed in PR #5374 (chore/build-error-types), but no Logs-specific structure found in public API.

Citations:


🏁 Script executed:

# First, let's check if the file exists and its size
wc -l src/commands/deploy/deploy.ts

Repository: netlify/cli

Length of output: 89


🏁 Script executed:

# Read the specific section mentioned in the review (lines 715-745 for context)
sed -n '715,745p' src/commands/deploy/deploy.ts | cat -n

Repository: netlify/cli

Length of output: 1149


🏁 Script executed:

# Look for the Logs type import and definition
rg "import.*Logs|type Logs|interface Logs" src/commands/deploy/deploy.ts -A 3 -B 3

Repository: netlify/cli

Length of output: 37


🏁 Script executed:

# Check what `@netlify/build` exports for Logs type
rg "export.*Logs" --type ts --type tsx

Repository: netlify/cli

Length of output: 82


🏁 Script executed:

# Search for runBuild function and Logs type definition
rg "runBuild|type Logs|interface Logs" src/commands/deploy/deploy.ts -B 2 -A 2

Repository: netlify/cli

Length of output: 1188


🏁 Script executed:

# Look for imports in the file
head -50 src/commands/deploy/deploy.ts | grep -E "^import|^from"

Repository: netlify/cli

Length of output: 1745


🏁 Script executed:

# Search for Logs type across the codebase
fd -e ts -e tsx . src | xargs rg "type Logs|interface Logs" -A 5 | head -50

Repository: netlify/cli

Length of output: 471


🏁 Script executed:

# Now let's check the Logs type definition in src/lib/build.ts
sed -n '1,30p' src/lib/build.ts | cat -n

Repository: netlify/cli

Length of output: 1313


🏁 Script executed:

# Check the actual Logs type from `@netlify/build` by examining how it's used elsewhere
rg "logs\." src/lib/build.ts -B 2 -A 2

Repository: netlify/cli

Length of output: 37


🏁 Script executed:

# Look at the runBuild return type
rg "function runBuild|export.*runBuild" src/lib/build.ts -A 10

Repository: netlify/cli

Length of output: 430


🏁 Script executed:

# Let's get more context on the Logs type usage and understand the pattern better
rg "logs\?" src/commands/deploy/deploy.ts -B 1 -A 1

Repository: netlify/cli

Length of output: 229


🏁 Script executed:

# Check if there's type definition or any hints about stdout/stderr optionality
rg "stdout|stderr" src/lib/build.ts -B 2 -A 2 | head -40

Repository: netlify/cli

Length of output: 237


🏁 Script executed:

# Get more precise line numbers for the exact code snippet
grep -n "When --verbose is used with --json" src/commands/deploy/deploy.ts

Repository: netlify/cli

Length of output: 134


🏁 Script executed:

# Get the exact lines around the code in question
sed -n '715,750p' src/commands/deploy/deploy.ts | cat -n

Repository: netlify/cli

Length of output: 1370


Add optional chaining to logs.stdout and logs.stderr for consistency and safety.

The code at lines 736 and 740 uses optional chaining (logs?.stdout.length, logs?.stderr.length), but the new code at lines 724 and 727 does not. Even though logs is checked for truthiness, the pattern used elsewhere in the same function suggests these properties may be undefined in some cases. Add optional chaining for consistency.

🛡️ Proposed fix
   if (options.verbose && options.json && logs) {
-    if (logs.stdout.length) {
+    if (logs.stdout?.length) {
       process.stderr.write(logs.stdout.join('\n'))
     }
-    if (logs.stderr.length) {
+    if (logs.stderr?.length) {
       process.stderr.write(logs.stderr.join('\n'))
     }
   }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (options.verbose && options.json && logs) {
if (logs.stdout.length) {
process.stderr.write(logs.stdout.join(''))
}
if (logs.stderr.length) {
process.stderr.write(logs.stderr.join(''))
}
}
if (options.verbose && options.json && logs) {
if (logs.stdout?.length) {
process.stderr.write(logs.stdout.join(''))
}
if (logs.stderr?.length) {
process.stderr.write(logs.stderr.join(''))
}
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/commands/deploy/deploy.ts` around lines 723 - 730, The conditional writes
to stderr should use optional chaining for the logs properties to match the rest
of the function: replace checks of logs.stdout.length and logs.stderr.length
with logs?.stdout?.length and logs?.stderr?.length (within the block where
options.verbose && options.json && logs is true) so that accessing potentially
undefined stdout/stderr is safe and consistent with existing uses of
logs?.stdout and logs?.stderr.


// Without this, the deploy command fails silently
if (exitCode !== 0) {
let message = ''
Expand Down
42 changes: 42 additions & 0 deletions tests/integration/commands/deploy/deploy.test.ts
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import path from 'path'
import { fileURLToPath } from 'url'

import execa from 'execa'
import { describe, expect, test } from 'vitest'

import { callCli } from '../../utils/call-cli.js'
import { cliPath } from '../../utils/cli-path.js'
import { getCLIOptions, type MockApi } from '../../utils/mock-api-vitest.js'
import { withSiteBuilder } from '../../utils/site-builder.js'
import { createDeployRoutes, startDeployMockApi, type DeployRouteState } from './deploy-api-routes.js'
Expand Down Expand Up @@ -699,6 +701,46 @@ describe.concurrent('deploy command', () => {
})
})

test('should pipe build output to stderr when --json --verbose is used on successful deploy', async (t) => {
await withMockDeploy(async (mockApi) => {
await withSiteBuilder(t, async (builder) => {
builder
.withContentFile({
path: 'public/index.html',
content: '<h1>Test content</h1>',
})
.withNetlifyToml({
config: {
build: {
publish: 'public',
command:
"node -e \"process.stdout.write('Build stdout line 1\\nBuild stdout line 2'); process.stderr.write('Build stderr line 1\\nBuild stderr line 2')\"",
},
},
})

await builder.build()

const { env, ...execOptions } = getCLIOptions({ apiUrl: mockApi.apiUrl, builder })
const { stdout, stderr } = await execa.node(cliPath, ['deploy', '--json', '--verbose'], {
...execOptions,
env,
timeout: 3e5,
nodeOptions: [],
})

// stdout should still be valid JSON
expect(() => JSON.parse(stdout)).not.toThrowError()

// stderr should contain the build output with line breaks preserved
expect(stderr).toContain('Build stdout line 1')
expect(stderr).toContain('Build stdout line 2')
expect(stderr).toContain('Build stderr line 1')
expect(stderr).toContain('Build stderr line 2')
})
})
})

test('should deploy hidden public folder but ignore hidden/__MACOSX files', async (t) => {
await withMockDeploy(async (mockApi, deployState) => {
await withSiteBuilder(t, async (builder) => {
Expand Down
Loading