Skip to content

chore: start server processes with node inspector#2361

Merged
liamdebeasi merged 11 commits into
mainfrom
ld/debugger
Jul 23, 2026
Merged

chore: start server processes with node inspector#2361
liamdebeasi merged 11 commits into
mainfrom
ld/debugger

Conversation

@liamdebeasi

Copy link
Copy Markdown
Contributor

Starts API servers with --inspect or --inspect-brk flag so breakpoints can be used to debug the server.

@github-actions

github-actions Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Here's a visual recap of what changed:

Visual recap

Open the full interactive recap

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@liamdebeasi
liamdebeasi marked this pull request as ready for review July 23, 2026 18:16
@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@netlify

This comment has been minimized.

@liamdebeasi
liamdebeasi requested review from a team and timmilazzo and removed request for a team July 23, 2026 18:17
builder-io-integration[bot]

This comment was marked as outdated.

@netlify

This comment has been minimized.

builder-io-integration[bot]

This comment was marked as outdated.

builder-io-integration[bot]

This comment was marked as outdated.

@liamdebeasi
liamdebeasi enabled auto-merge (squash) July 23, 2026 20:06

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Builder reviewed your changes and found 1 potential issue 🟡

Review Details

Code Review Summary

This incremental review covers the unchanged latest PR head for Node inspector support in agent-native dev and the template script updates. The single-app implementation correctly separates Vite from the Nitro server, resolves Vite's JS entry, injects NODE_OPTIONS through a preload, and preserves the normal non-inspector path.

Key Findings

  • 🟡 Windows inspector startup can fail — the new data-URL preload is passed through the existing Windows shell: true path, where cmd.exe can expand the URL's percent-encoded sequences before Node receives it. This can corrupt the preload and prevent agent-native dev --inspect from starting on Windows.
  • 🟡 Workspace-root inspector propagation remains outstanding from the previous review; it is still covered by the existing open review comment and is not resubmitted.

Risk level: Standard. The rest of the updated single-app path had no additional confirmed issues.

🧪 Browser testing: Skipped — PR only modifies CLI/process startup and package configuration, with no user-facing UI impact.

Comment on lines +599 to +609
const preload =
"data:text/javascript," +
encodeURIComponent(
`process.env.NODE_OPTIONS=((process.env.NODE_OPTIONS??"")+" ${directive}").trim();`,
);
const env = {
...process.env,
NITRO_DEV_RUNNER: process.env.NITRO_DEV_RUNNER ?? "node-process",
};
console.log(`[agent-native] API server debugger listening on ${target}`);
run(process.execPath, ["--import", preload, viteJsEntry, ...rest], { env });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Avoid routing the encoded preload through cmd.exe on Windows

On Windows, run() sets shell: true, so this new invocation is serialized through cmd.exe. The percent-encoded data: URL contains %xx sequences from encodeURIComponent; cmd.exe can expand paired percent signs before Node receives the argument, corrupting the preload module URL and causing agent-native dev --inspect to fail. Launch this Node/Vite-entry path without a shell, or use a shell-safe preload representation, while preserving the existing shell behavior for the non-inspector Vite shim path.

Additional Info
Confirmed by one independent review agent; applies to the new inspector launch at lines 599-609.

Fix in Builder

@liamdebeasi

Copy link
Copy Markdown
Contributor Author

@builderio-bot can you investigate why src/a2a/client.spec.ts > A2AClient > bounds a hung task-status request by the overall poll deadline fails?

@builder-io-integration

Copy link
Copy Markdown
Contributor

Investigated. Root cause was in the test's hand-rolled fetch mock, not in A2AClient or the poll-deadline logic.

In both bounds a hung task-status request by the overall poll deadline and recovers after one task-status request exceeds the per-request timeout, the mock only registers an addEventListener("abort", ...) on the request signal — it never checks whether the signal is already aborted before adding that listener.

Under vi.useFakeTimers(), A2AClient.postJson arms its abort timer (setTimeout(() => controller.abort(), requestTimeoutMs)) before calling ssrfSafeFetch, which does real (unmocked) pre-flight work — a dynamic import("undici") and a real node:dns/promises lookup — before it ever reaches the mocked fetch. That pre-flight work takes enough real wall-clock time that, once the test advances the fake clock, the abort timer can fire before the mock's fetch is actually invoked. By the time the mock runs, signal.aborted is already true, but it only listens for a future abort event — which already fired — so the returned promise never settles. The test then hangs until Vitest's own 5000ms per-test timeout kills it, which is exactly what the CI logs show (a ~5s gap with no further test output before the next test's message/send starts).

This is unrelated to this PR's changes (which only touch the CLI dev-inspect-flag work) — it's a pre-existing flaky test from a same-day commit (c15d20f3).

Fix pushed to ld/debugger: the mocks now check init.signal?.aborted synchronously and reject immediately if already true, matching how a real fetch implementation behaves. Verified 5 consecutive full-file runs pass (24/24) after the change.

@builder-io-integration

Copy link
Copy Markdown
Contributor

Root cause found and fixed: the two hung-poll tests in client.spec.ts had a fetch mock that only listened for a future abort event but never checked if the signal was already aborted. Under fake timers, real (unmocked) DNS/import work inside ssrfSafeFetch let the abort timer fire before the mock's fetch was even invoked, so the mock's promise hung forever until Vitest's own 5s timeout killed the test — unrelated to PR #2361's actual changes. Pushed a fix to ld/debugger (verified 24/24 passing across 5 runs) and posted the explanation as a PR comment.



🟢 Fixed and pushed the flaky A2A client test; explained root cause in a PR comment.


Edit in Builder  Preview

@builder-io-integration builder-io-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Builder reviewed your changes — no new findings

Review Details

Code Review Summary

This incremental review covers the latest PR #2361 head, which adds the Node inspector launch path and updates the A2A abort-signal test mocks. The new A2A mock behavior correctly handles signals already aborted before listener registration while preserving the existing asynchronous abort behavior. The inspector implementation remains sound for single-app mode, including Vite entry resolution, Nitro runner selection, and environment propagation.

Key Findings

No new confirmed issues were found in the latest changes. The existing workspace-root inspector propagation and Windows cmd.exe preload concerns remain open and were intentionally not reposted. One reviewer also verified the A2A test suite passes.

Risk level: Standard.

🧪 Browser testing: Skipped — PR only modifies CLI/process startup, tests, and package configuration, with no user-facing UI impact.

@liamdebeasi
liamdebeasi merged commit 20ebb96 into main Jul 23, 2026
44 checks passed
@liamdebeasi
liamdebeasi deleted the ld/debugger branch July 23, 2026 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants