ci: verify harness runtime in Cloudflare Workers#127
Conversation
Wrangler is now part of the edge-safety signal: CI builds the harness package, dry-runs a Worker deployment, starts local workerd through wrangler dev, and fetches a fixture that imports @ai-sdk-tool/harness/runtime. The smoke caught optional Node-only MCP and skills modules being pulled into edge bundles, so those imports are now late string dynamic imports while keeping TypeScript type checking through typeof import(...). Constraint: Cloudflare Worker bundles must import the public runtime subpath without process, fs, child_process, or other Node-only globals. Rejected: Keep only the browser-condition Vitest check | it did not exercise Wrangler/workerd bundling and missed optional Node-only dependency edges. Confidence: high Scope-risk: moderate Directive: Do not replace the Wrangler smoke with a pure import test unless it still runs a real Worker bundle and request. Tested: CI=true pnpm install --frozen-lockfile Tested: pnpm run check Tested: pnpm run typecheck Tested: pnpm run test:edge-safe Tested: pnpm run build Tested: pnpm run test Not-tested: Remote Cloudflare deploy with account credentials or production bindings.
|
Important Review skippedAuto reviews are disabled on this repository. To trigger a review, include ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a Cloudflare Worker edge smoke test to verify runtime subpaths and ensure Node-only modules are excluded from edge bundles. Key changes include the addition of a smoke test worker, configuration files, and a test runner script. The review feedback highlights opportunities to improve the test runner script by properly managing temporary directories and log paths to prevent resource leaks and potential collisions during concurrent runs.
| @@ -0,0 +1,152 @@ | |||
| import { spawn, spawnSync } from "node:child_process"; | |||
| import { mkdtempSync } from "node:fs"; | |||
There was a problem hiding this comment.
Add rmSync to the imports from node:fs to enable cleanup of the temporary directory created for the smoke test.
import { mkdtempSync, rmSync } from "node:fs";| const WRANGLER_ENV = { | ||
| ...process.env, | ||
| FORCE_COLOR: "0", | ||
| WRANGLER_LOG_PATH: join(tmpdir(), "plugsuits-wrangler-logs"), | ||
| WRANGLER_SEND_METRICS: "false", | ||
| }; | ||
| const WORK_DIR = mkdtempSync(join(tmpdir(), "plugsuits-worker-edge-")); |
There was a problem hiding this comment.
The WRANGLER_LOG_PATH currently uses a fixed filename in the system temp directory, which can lead to collisions if multiple smoke tests run concurrently (e.g., in different CI jobs or local runs). It is safer to place the logs inside the unique WORK_DIR. Reordering these constants ensures WORK_DIR is available when defining WRANGLER_ENV.
const WORK_DIR = mkdtempSync(join(tmpdir(), "plugsuits-worker-edge-"));
const WRANGLER_ENV = {
...process.env,
FORCE_COLOR: "0",
WRANGLER_LOG_PATH: join(WORK_DIR, "wrangler.log"),
WRANGLER_SEND_METRICS: "false",
};| } finally { | ||
| await stopChild(child); | ||
| } |
There was a problem hiding this comment.
The temporary directory WORK_DIR is created but never deleted. It should be cleaned up in the finally block to avoid leaving orphaned files on the system or CI runner after the test completes.
} finally {
await stopChild(child);
rmSync(WORK_DIR, { recursive: true, force: true });
}![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/cloudflare-worker-edge-smoke/run.mjs">
<violation number="1" location="scripts/cloudflare-worker-edge-smoke/run.mjs:144">
P2: Selecting a port by binding/closing it before starting `wrangler dev` introduces a race that can make this smoke test flaky if another process takes the port.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The smoke runner previously opened an ephemeral port and closed it before starting wrangler dev on that same numeric port. That made the chosen port advisory rather than reserved, so another process could take it between close and bind and make the CI check flaky. Constraint: The smoke test must run as a real local Wrangler/workerd server without requiring a fixed port. Rejected: Retry on EADDRINUSE | this still leaves the race in place and adds slower failure handling. Confidence: high Scope-risk: narrow Directive: Let Wrangler bind port 0 and consume the emitted ready URL instead of pre-selecting a local port. Tested: pnpm run check Tested: pnpm run test:edge-safe Not-tested: Remote Cloudflare deploy with account credentials.
|
@cubic review |
|
@codex review |
There was a problem hiding this comment.
1 issue found across 9 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/cloudflare-worker-edge-smoke/run.mjs">
<violation number="1" location="scripts/cloudflare-worker-edge-smoke/run.mjs:57">
P2: Add a per-request timeout to the smoke-test fetch so the 30s polling deadline cannot be bypassed by a hanging response.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/cloudflare-worker-edge-smoke/run.mjs">
<violation number="1" location="scripts/cloudflare-worker-edge-smoke/run.mjs:169">
P1: `readyUrl` is awaited without a timeout, so the smoke script can hang indefinitely if wrangler never emits a matching readiness log line.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
The smoke runner already had a 30s polling loop, but the awaited operations inside it were not individually bounded. A hung response could keep a single fetch/json read past the polling deadline, and wrangler dev could keep running forever if it stayed alive without emitting the expected ready URL. Constraint: CI needs a bounded failure path even when wrangler output or the local HTTP response is malformed or incomplete. Rejected: Rely on the outer polling deadline only | awaited fetch/json and readiness promises can bypass that deadline. Confidence: high Scope-risk: narrow Directive: Keep all long-running external waits in this smoke runner behind explicit timeouts. Tested: pnpm run check Tested: pnpm run test:edge-safe Not-tested: Remote Cloudflare deploy with account credentials.
|
@cubic review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d743a0d25d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| DefinedAgent, | ||
| } from "./types"; | ||
|
|
||
| const SKILLS_MODULE = "../skills.js"; |
There was a problem hiding this comment.
The new "../skills.js" specifier in source TypeScript violates the repo rule in AGENTS.md (root) that source imports must stay extensionless, and it can break @ai-sdk-tool/source consumers that execute/compile TS sources directly (where skills.js does not exist alongside skills.ts), causing runtime resolution failures when definition.skills is enabled. Please keep the source specifier extensionless and handle .js rewriting only in build output.
Useful? React with 👍 / 👎.
| } | ||
| export type StopCondition = (input: StopConditionInput) => boolean; | ||
|
|
||
| const MCP_INIT_MODULE = "./mcp-init.js"; |
There was a problem hiding this comment.
This change introduces "./mcp-init.js" in source TS, which conflicts with the root AGENTS.md guideline requiring extensionless source imports and risks module-not-found errors for source-condition users/toolchains that don’t map .js back to .ts; that failure path is triggered specifically when config.mcp is provided. Keep the source import extensionless and apply extension fixes in compiled dist instead.
Useful? React with 👍 / 👎.
1 participant
Summary
test:edge-safeto build@ai-sdk-tool/harness, runwrangler deploy --dry-run, start localwrangler dev, and fetch a Worker fixture importing@ai-sdk-tool/harness/runtime.edge-safeGitHub Actions job for the Wrangler/Cloudflare Worker smoke.@ai-sdk-tool/harness.Why this is not duplicate coverage
The existing
runtime-edge-importsVitest check only exercises browser-condition importing. This Worker smoke asks Wrangler to bundle the public runtime export, then serves it through local workerd and verifies a real fetch response. This caught the Node-only optional import edges locally before the fix.Verification
CI=true pnpm install --frozen-lockfilepnpm run checkpnpm run typecheckpnpm run test:edge-safepnpm run buildpnpm run testNotes
test:edge-saferequires local loopback access becausewrangler devstarts workerd on127.0.0.1.Summary by cubic
Adds a Cloudflare Worker smoke test to verify
@ai-sdk-tool/harness/runtimebundles and runs under Workers, and makes the CI check more reliable with bounded waits.New Features
test:edge-safebuilds@ai-sdk-tool/harness, runswrangler deploy --dry-run, starts localwrangler dev, and fetches a Worker fixture.edge-saferuns the smoke test locally withworkerd; no Cloudflare account needed.Bug Fixes
wrangler devnow binds port 0 and the runner uses the emitted ready URL.Written for commit d743a0d. Summary will update on new commits. Review in cubic